Gitlab Pull Requests

[DLu]

• Enable making pull requests to Gitlab-based rosdistro files (instead of just allowing github.com)
  • This makes use of the python-gitlab library. Not sure if python-bloom minds having that dependency since it doesn't use the github lib.
  • Separates out the logic for reading the configuration from ~/.config/bloom into get_bloom_config_and_path
  • Moves the logic for creating the title/body of the pull request earlier in the open_pull_request
  • Lots of whitespace changes because the github logic is behind an if statement.

[nuclearsandwich]

Thanks for the PR. Below are my thoughts on the functionality listed in the body rather than comments on the specific implementation, which I haven't looked at in detail yet.

I think the two auxiliary features would benefit from separate pull requests as they are features that can be considered independently.

• Enables the use of a new environment variable BLOOM_RELEASE_REPO_BASE.
• This PR also makes one small change that has always bugged me. It makes the track command line argument optional, and if it is not set, it sets it to the rosdistro.

This makes use of the python-gitlab library. Not sure if python-bloom minds having that dependency since it doesn't use the github lib.

Hopefully GitLab's pull request API doesn't require more than a small handful of https calls. I think it's reasonable create a helper module analogous to github.py to avoid pulling in a heavy dependency for a very narrow subset of its functionality.

[DLu]

Updated this PR and created #460 and #459

Is the dependency so heavy such that its really worth it to reimplement all the calls in this code? Seems like more to maintain here. The github code isn't exactly straightforward.

[wjwwood]

The only concern with a dependency is if there a debian package for it that we can depend on.

[DLu]

https://packages.debian.org/search?keywords=python-gitlab Does this count? I honestly don't know how this works.

[nuclearsandwich]

The only concern with a dependency is if there a debian package for it that we can depend on.

https://packages.debian.org/search?keywords=python-gitlab Does this count? I honestly don't know how this works.

That's the right package for Debian, and it's in the Ubuntu repositories but only starting from Xenial which means we cannot release it in bloom until we drop support for older distros (trusty, at least). There is also a python3 package, (python3-gitlab) which will be needed for those who use bloom with python3.

The pypi package python-gitlab has no apparent associated Debian/Ubuntu package.

Is the dependency so heavy such that its really worth it to reimplement all the calls in this code? Seems like more to maintain here. The github code isn't exactly straightforward.

There are definitely improvements that could be made in the GitHub logic. I think the fact that it sits in this repository and implements only the functionality required by bloom makes active maintenance of the GitHub functionality easier than it would be if we pulled in a library that managed the GitHub API interactions for us and had to deep dive into that whenever it didn't Just Work for us.

How did you install the gitlab dependency locally @DLu?

Looking more closely at the gitlab python module itself. This PR adds python-gitlab to setup.py, but that pypi package is not related to the Debian/Ubuntu package python-gitlab. The corresponding pypi package is pyapi-gitlab. The version in all debian-derived distros is 7.5.0 while the latest pypi release is 7.8.5. Per the documentation for that package, this indicates that it only supports the GitLab API for GitLab 7.5 and earlier. Which versions of GitLab has this been tested on?

Assuming that the above availability concerns can be resolved, my general concerns with the addition of these dependencies are:

1. GitLab (and GitHub, BitBucket, etc) pull request generation is a feature that is not used by all bloom users. By that logic, there should be no required dependencies linked with any one of them. We have more leeway with bloom because it's not a tool I would expect to need in a runtime ROS environment. If it was, this objection would matter materially rather than just in principle as the extraneous dependencies would bloat a runtime install.

2. Bloom is a ground-level ROS ecosystem tool. While we support it primarily on ROS's supported platforms, I think we also have a responsibility not to impede community porting efforts it to new platforms. And that includes keeping dependencies manageable. Again, this concern is diminished by the fact that bloom is also available via the Python Package Archive for any platform which supports that, but the official ROS release channels prefer and recommend platform package management.

[wjwwood]

We could introduce this as an optional dependency, adding it in the setup.py but not the Debian package. But that would take changes to this pr so that it could function even in the absence of the gitlab python module you're using. So if you installed it from the debian, it would tell you that gitlab isn't supported without the need to manually install the gitlab module.

[DLu]

Would this commit address your concerns?

[nuclearsandwich]

Would this commit address your concerns?

Just looking at the diff it looks like it would indeed. I've got to get a GitLab rosdistro set up to test the functionality. Out of curiosity have you been using either of these branches with python2 or python3? Just wondering where I should focus my own testing.

[nuclearsandwich]

@DLu Thanks for taking a crack at an internal gitlab interaction library. I certainly still think it's the best option for Bloom as it is going to be much easier for us to maintain and distribute.

I pulled e13e748 into a local workspace and took it for a spin. There were some issues with the changes including python2/3 compatibility issues and the usage of requests instead of standard library http. Are you interested in resolving those issues or should I pull your patch into a local branch as is and start working on it?

Regarding requests, I'm warming on adding it as a dependency. The version spread from Trusty - Bionic is 2.2 - 2.18. As long as we do some testing with bloom on all supported platforms before cutting a final release it could be fine.

[DLu]

I am happy to work on this further. If you notate the problems you found in PR Review form, I can try to pick them off.

[nuclearsandwich]

I am happy to work on this further. If you notate the problems you found in PR Review form, I can try to pick them off.

Can you update the PR branch to base it on the version without the gitlab library dependency and ideally rebase or replay your patches on top of the latest version of master. That will give me a good basis for online review.

[DLu]

Squashed into a single commit and rebased.

[DLu]

Closing in favor of #500