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

Gitlab library #490

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

DLu
Copy link
Contributor

@DLu DLu commented Sep 29, 2018

This PR has not been tested in isolation, but is seemingly valid Python.

This is mostly to debate whether or not I'm allowed to use Python requests. The conversation was previously here where @nuclearsandwich said they were warming to the new dependency, but I wanted to reopen that conversation.

@nuclearsandwich
Copy link
Contributor

This is mostly to debate whether or not I'm allowed to use Python requests.

Short answer: Green light. Please do some Trusty testing.

Rationale: Requests is available via system package managers in every platform supported by rosdep and Alpine where rosdep support is still in progress. bloom itself doesn't leverage the rosdep database for its dependencies but it's a good enumeration of ROS's possible target platforms.
My primary concern with adding dependencies to ROS infrastructure packages is that it increases the theoretical overhead of porting ROS to a new platform. Bloom is a bit of a special case in that regard as it is run primarily on developer workstations rather than as part of a "live" ROS installation. Additionally, Requests is well tested, widely used, and available via pip which is how most new ROS platforms will likely start building their ROS toolchain, including bloom.

Please be sure to set the requests minimum version to 2.2 in setup.py to signify that it was the oldest tested and update the stdeb.cfg dependencies.

@DLu
Copy link
Contributor Author

DLu commented Oct 11, 2018

I still need to do the requested Trusty testing on this, but please check that I added the dependencies properly.

@DLu
Copy link
Contributor Author

DLu commented Nov 5, 2018

@nuclearsandwich I finally had some time to install trusty and check things here. requests >= 2.2 is valid there, and the CI is now passing.

@DLu DLu mentioned this pull request Dec 4, 2018
@nuclearsandwich
Copy link
Contributor

@DLu I know I mentioned privately that I had no reservations about this PR. And that's still largely the case, but what was the reason for removing the requests module from setup.py? I believe it will still be needed there for when bloom is installed via pip / from source.

@DLu
Copy link
Contributor Author

DLu commented Jan 11, 2019

Sorry for the delay.

I'm not sure how setup.py really works, but with requests module in setup.py, it broke the CI

Without it, it passes.

🤷‍♂️

@nuclearsandwich
Copy link
Contributor

That's something I'll have to investigate then. I'm pretty certain it needs to be there for users who install bloom via pip.

@DLu
Copy link
Contributor Author

DLu commented Dec 13, 2019

@nuclearsandwich Is this something there is still interest in merging in?

@DLu
Copy link
Contributor Author

DLu commented Jun 10, 2020

With the advent of Tailor I don't need this anymore, but I've updated it if others think there is sufficient use for Gitlab.

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