Skip to content

Conversation

@DLu
Copy link
Contributor

@DLu DLu commented Feb 5, 2018

  • Enables the use of a new environment variable BLOOM_RELEASE_REPO_BASE.
    • This can be used instead of manually inputting the release repo url.
    • If the environment var exists, and BLOOM_RELEASE_REPO_BASE + repository + '-release.git' exists as a repository, then it will use that as the release repo.
    • For example, all my release repos are over at https://github.com/wu-robotics/ so if I set that as my environment variable, I can create a bloom-example-release repo there, and then if I try to bloom-release a package called bloom-example, it will automagically use that as my release repo.

@DLu DLu mentioned this pull request Feb 5, 2018
Copy link
Contributor

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I guess this is fine, but honestly I don't think it should be needed most of the time. If the package has ever been released on a previous ROS distribution, then it will guess it from the previous one. If it's already been released into the current ROS distro, then it doesn't need the url (it will use the one from the previous release).

So this only applies on the first release of a package ever in any ros distribution. But if you find it useful, then that's fine I guess.

@nuclearsandwich
Copy link
Contributor

I know bloom's documentation isn't in the best of shape, but I would love to see this new feature documented somewhere so I can add documentation for more of the environment variables used by Bloom.

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

I would like to see at least a docstring on the added function. If you're really feeling your oats and want to add a unit test or a section in the docs I would be very grateful. But I don't think either would necessarily block the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a docstring to this function describing the environment variable's behavior.

@DLu
Copy link
Contributor Author

DLu commented Feb 7, 2018

I would be willing to add a test or some documentation, but I'm unclear where it would go best. I'm not familiar with the test structures in this package, and it seems like the documentation should just go on the wiki.

@nuclearsandwich
Copy link
Contributor

Thanks for the docstring DLu. I'll admit that I'm also getting familiar with the test structures so I don't have much concrete advice for you on that front.

I'm going to merge this so we can add support in the next release.

@nuclearsandwich nuclearsandwich merged commit feb9001 into ros-infrastructure:master Feb 28, 2018
@DLu DLu deleted the feature/env_var branch March 1, 2018 14:02
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.

3 participants