Skip to content

Update rosdistro to track latest released distro#242

Merged
ruffsl merged 4 commits intomasterfrom
ruffsl-patch-1
Mar 22, 2019
Merged

Update rosdistro to track latest released distro#242
ruffsl merged 4 commits intomasterfrom
ruffsl-patch-1

Conversation

@ruffsl
Copy link
Copy Markdown
Member

@ruffsl ruffsl commented Mar 20, 2019

No description provided.

Copy link
Copy Markdown
Contributor

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm

- --from-paths src
- --ignore-src
- --rosdistro bouncy
- --rosdistro crystal
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could we make the distro name a parameter that we will use on this line as well as for constructing the URL L49?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean. Set ENV ROS_DISTRO and use that to ${} substitute in the githubusercontent.com/ros2/ros2/ URL? I thought we'd just pass the URL from the config outright so that users may point to custom gists and the like.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's a nitpick, I'm fine merging as is, I'm mostly trying to look for ways to avoid mismatches in the future


This PR was necessary because there was a mismatch between the distro used by rosdep and the repos file pulled.

If we used a url with a format like https://raw.githubusercontent.com/ros2/ros2/@ros2distro/ros2.repos and rosdep with something like:

            install:
                - --from-paths src
                - --ignore-src
                - --rosdistro @ros2distro

We would be ensured that the invocations match in the future.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think that is good idea. Please see b6d5cde and a8955e9 .
I could update the template accordingly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good to me 👍 thanks for iterating

ruffsl added 2 commits March 20, 2019 11:55
and have it be used in following setup for consistency
@mikaelarguedas
Copy link
Copy Markdown
Contributor

with osrf/docker_templates#53 and osrf/docker_templates#56 this should be good to go

@ruffsl ruffsl merged commit bd344e1 into master Mar 22, 2019
@mikaelarguedas mikaelarguedas deleted the ruffsl-patch-1 branch March 22, 2019 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants