Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

simplify Setup ROS 2 with devcontainer. #4161

Merged
merged 3 commits into from
Mar 11, 2024
Merged

simplify Setup ROS 2 with devcontainer. #4161

merged 3 commits into from
Mar 11, 2024

Conversation

fujitatomoya
Copy link
Collaborator

address #3664

see #3664 (comment)

@fujitatomoya fujitatomoya force-pushed the fujitatomoya/ros2-dev-container-env branch from e3a9993 to 584d407 Compare February 12, 2024 19:12
@fujitatomoya
Copy link
Collaborator Author

@vkuehn @rlekkerkerker-hva @bgigous could you take a look at this fix? just tying to make it more simplified without complication.

@vkuehn
Copy link

vkuehn commented Feb 12, 2024

simplification makes sense as far I can see. unfortunatetly having currently no chance to rework on that

@bgigous
Copy link

bgigous commented Feb 12, 2024

Was it intended that this line be removed? I think it looks good otherwise.

rlekkerkerker-hva
rlekkerkerker-hva approved these changes Feb 14, 2024
View reviewed changes
Copy link

@rlekkerkerker-hva rlekkerkerker-hva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I'm not the expert here, because I came here to learn, but it makes more sense when I read the new version. Thanks!

@fujitatomoya fujitatomoya changed the title Fujitatomoya/ros2 dev container env simplify Setup ROS 2 with devcontainer. Feb 14, 2024
@fujitatomoya
address review comments.
4ef9410 
Signed-off-by: Tomoya.Fujita <[email protected]>
@fujitatomoya
Copy link
Collaborator Author

Was it intended that this line be removed? I think it looks good otherwise.

right, this should stay. my bad... fixed.

@fujitatomoya
Copy link
Collaborator Author

@clalancette @audrow can you take a look when you have time?

clalancette
clalancette requested changes Mar 11, 2024
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about this. Overall, it looks good to me but I've got one open comment.

source/How-To-Guides/Setup-ROS-2-with-VSCode-and-Docker-Container.rst Outdated
@@ -201,14 +189,14 @@ Open the Dockerfile and add the following contents:
USER $USERNAME
CMD ["/bin/bash"]

Search here also for ``ROS_DISTRO`` with the ROS 2 distribution you wish to use and added to the cache previously.
Search here also for ``ROS_DISTRO`` with the ROS 2 distribution you wish to use.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear to me why you would "search" here for the ROS_DISTRO. You search and then do...what?

(this was an existing problem with this sentence)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added Replace... sentence for this, how does it look? 611df1f

besides it, caching directories are removed, and removed that sentence as well.

@fujitatomoya
address review comments.
611df1f 
Signed-off-by: Tomoya.Fujita <[email protected]>
@clalancette clalancette added the backport-all backport at reviewers discretion; from rolling to all versions label Mar 11, 2024
@clalancette clalancette merged commit d3b27f5 into rolling Mar 11, 2024
4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fujitatomoya/ros2-dev-container-env branch March 11, 2024 15:39
mergify bot pushed a commit that referenced this pull request Mar 11, 2024
@fujitatomoya @mergify
simplify Setup ROS 2 with devcontainer. (#4161)
64b4c35 
* simplify Setup ROS 2 with devcontainer.

Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit d3b27f5)
mergify bot pushed a commit that referenced this pull request Mar 11, 2024
@fujitatomoya @mergify
simplify Setup ROS 2 with devcontainer. (#4161)
2d6c7a7 
* simplify Setup ROS 2 with devcontainer.

Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit d3b27f5)
This was referenced Mar 11, 2024
Merged
Merged
clalancette pushed a commit that referenced this pull request Mar 11, 2024
@mergify @fujitatomoya
simplify Setup ROS 2 with devcontainer. (#4161) (#4219)
03a049d 
* simplify Setup ROS 2 with devcontainer.

Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit d3b27f5)

Co-authored-by: Tomoya Fujita <[email protected]>
clalancette pushed a commit that referenced this pull request Mar 11, 2024
@mergify @fujitatomoya
simplify Setup ROS 2 with devcontainer. (#4161) (#4220)
4364356 
* simplify Setup ROS 2 with devcontainer.

Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit d3b27f5)

Co-authored-by: Tomoya Fujita <[email protected]>
@fujitatomoya fujitatomoya mentioned this pull request Sep 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@rlekkerkerker-hva rlekkerkerker-hva rlekkerkerker-hva approved these changes

@audrow audrow Awaiting requested review from audrow

Assignees

@fujitatomoya fujitatomoya

Labels
backport-all backport at reviewers discretion; from rolling to all versions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fujitatomoya @vkuehn @bgigous @clalancette @rlekkerkerker-hva