Skip to content

Locus Specific Bloom Features#3

Merged
DLu merged 6 commits intolocus-masterfrom
feature/locus_awesome
Feb 5, 2018
Merged

Locus Specific Bloom Features#3
DLu merged 6 commits intolocus-masterfrom
feature/locus_awesome

Conversation

@DLu
Copy link
Contributor

@DLu DLu commented Feb 1, 2018

(plus one thing that just annoys me)

  • Opens pull merge requests for gitlab!
  • Will automatically attempt to use the http://gitlab-ci-token:TOKEN@gitlab.locusbots.io/locusrobotics/REPOSITORY-release.git as the release repository if it exists.
  • If not specified, assumes that track is the same as rosdistro

The gitlab portions will only work if you have pip install python-gitlab. The github portions use a custom interface to the api (i.e. not pygithub or similar). I don't know if that's to avoid the dependency. Furthermore, the gitlab portions are probably not ready to be merged to upstream since additional logic is probably needed for seeking out the right token, rather than assuming it is installed in ~/.git-tokens

@DLu DLu requested review from ayrton04 and paulbovbel February 1, 2018 21:25
@DLu
Copy link
Contributor Author

DLu commented Feb 1, 2018

P.S. I've made two successful PRs with this code.
e.g. http://gitlab.locusbots.io/locusrobotics/rosdistro/merge_requests/10

Copy link
Member

@paulbovbel paulbovbel left a comment

Choose a reason for hiding this comment

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

LGTM, pending the requirement change.

If you wanted to take this upstream and reduce the fork factor, you could remove the locus-convenience stuff (maybe have some kind of opt-in configuration options for repository url templating), and detect a gitlab rosdistro (maybe if 'gitlab' in server).

return _gl
# Make sure we can import gitlab
try:
import gitlab
Copy link
Member

Choose a reason for hiding this comment

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

I think you can safely import this up top if you add it to https://github.com/locusrobotics/bloom/blob/locus-master/setup.py#L6, that should ensure dev machines pull the library down as well.

Copy link
Member

@ayrton04 ayrton04 left a comment

Choose a reason for hiding this comment

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

Wow! That's really handy. So now it behaves as it does when you bloom from a GitHub-based rosdistro?

@DLu
Copy link
Contributor Author

DLu commented Feb 2, 2018

@ayrton04 Pretty much, modulo where the credentials are saved and how the API is accessed.

@ayrton04 @paulbovbel
What if there was an environment variable that was BLOOM_RELEASE_REPO_BASE that we could set to http://gitlab-ci-XXXX@gitlab.locusbots.io/locusrobotics/ and I personally might set to https://github.com/wu-robotics/ and then $REPOSITORY-release.git would be appended onto the end. Does that sound reasonable?

Then we could store the gitlab access key in .config/bloom like the github access stuff is, and remove the special git-tokens variable.

I should note that the one other thing I did in the locus-bloom script was to make sure the github token was prepended onto the URLs that go into rosdistro. Given your familiarity with the build farm, how important do you consider that?

@paulbovbel
Copy link
Member

paulbovbel commented Feb 2, 2018

That all sounds very reasonable, especially if just sets the default and can be overriden during the --edit phase. You may want to consider just dumping more keys into ~/.config/bloom rather than dealing with environments.

If this is just for us, then ++1, if it's for upstream, you may just want to make a PR and work through it with '@'nuclearsandwich - I'm sure upstream would love to enable gitlab MRs, but they have a lot more use cases to consider than we do.

@DLu
Copy link
Contributor Author

DLu commented Feb 5, 2018

The last two commits remove the git-tokens Locus-specific logic, and updates the gitlab interaction to be more in the same style of the github interaction. Also removes our specific patterns in favor of environment vars. Please take another look.

return config, oauth_config_path



Copy link
Member

Choose a reason for hiding this comment

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

Is this one too many line breaks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ERROR: Too many linting errors for me to notice this one.

Thanks.

@DLu DLu merged commit 4d24e13 into locus-master Feb 5, 2018
@DLu DLu deleted the feature/locus_awesome branch February 5, 2018 15:46
@DLu
Copy link
Contributor Author

DLu commented Feb 5, 2018

Thank you. PR opened upstream ros-infrastructure/bloom#458

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