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.

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

[doc] Clarify the semi-deprecated ROS_REPOSITORY_PATH env var usage #132

Merged
merged 1 commit into from
Mar 16, 2017

Conversation

130s
Copy link
Member

@130s 130s commented Feb 23, 2017

Found at #131 (comment)

I don't know if this is what we wanted/want. Thoughts?

If we decide to merge this change then we might want to inform users because this changes the default behavior.

@130s 130s mentioned this pull request Feb 23, 2017
@mathias-luedtke
Copy link
Member

I would not merge it by now, but I think shadow should not be the default.
I really like the moveit_ci ROS_REPO variable, it would be great to have some shortcuts like shadow, shadow-fixed, stable, testing, building etc.

ROS_REPO and ROS_REPOSITORY_PATH won't be used with custom Docker images anyway (as the repo setup is part of the image).

We could implement the follwing logic:
If ROS_REPOSITORY_PATH is set, use it.
If not, try to derive it from ROS_REPO (which defaults to stable).

@130s
Copy link
Member Author

130s commented Feb 23, 2017

I really like the moveit_ci ROS_REPO variable, it would be great to have some shortcuts like shadow, shadow-fixed, stable, testing, building etc.

You mean these moveit_ci. Indeed they look nice.

If ROS_REPOSITORY_PATH is set, use it.
If not, try to derive it from ROS_REPO (which defaults to stable).

+1

@mathias-luedtke
Copy link
Member

Should we reissue this PR?

@mathias-luedtke
Copy link
Member

Why did you replace ROS_REPOSITORY_PATH in the second commit?
This will fail!

@mathias-luedtke
Copy link
Member

The new deafult can be set there

@130s
Copy link
Member Author

130s commented Mar 13, 2017

Sorry, I was confused.

Added a bit more of the relationship of ROS_REPOSITORY_PATH and ROS_REPO to the document.

@130s
Copy link
Member Author

130s commented Mar 14, 2017

Conflict resolved.

Copy link
Member

@mathias-luedtke mathias-luedtke left a comment

Choose a reason for hiding this comment

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

Please don't set ROS_REPOSITORY_PATH as this will disable the ROS_REPO feature.
Default must be set there

@130s 130s changed the title [env] Use ROS official binary repo by default, instead of shadow-fixed repo. [doc] Clarify the semi-deprecated ROS_REPOSITORY_PATH env var usage Mar 15, 2017
@130s
Copy link
Member Author

130s commented Mar 15, 2017

Please don't set ROS_REPOSITORY_PATH as this will disable the ROS_REPO feature.

I see, done (by simply resetting the commit that did wrong).
Title of the PR also updated now that this only changes a doc.

@mathias-luedtke
Copy link
Member

Should I open another PR for the new default repo?

@mathias-luedtke mathias-luedtke merged commit 6aadbe5 into ros-industrial:master Mar 16, 2017
@130s 130s deleted the impr/defaultrepo branch December 10, 2017 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants