-
Notifications
You must be signed in to change notification settings - Fork 67
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
Adding Bitbucket Server integration for pyup #349
base: master
Are you sure you want to change the base?
Conversation
* Added unittests * Added provider * Modified setup and cli to fit the added provider
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #349 +/- ##
==========================================
+ Coverage 93.92% 94.05% +0.13%
==========================================
Files 11 12 +1
Lines 1103 1245 +142
==========================================
+ Hits 1036 1171 +135
- Misses 67 74 +7 ☔ View full report in Codecov by Sentry. |
Small bump @Jwomers 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work here. It would be really great supporting BitBucket servers. I would say my main concern is which server are we supporting with this. Atlassian merged its branding of BitBucket Server, BitBucket Cloud and Stash. So, I am wondering which one this supports.
My second point relates to the base-url in the user-token. It is OK to use a token with user@app-password format or something like that, but the URL has its own place to go as far as I understand. Also, the URL could default to BitBucket cloud, just in case.
Looking forward your feedback on this review. 👍
@@ -4,6 +4,7 @@ | |||
from pyup.requirements import RequirementFile, RequirementsBundle | |||
from pyup.providers.github import Provider as GithubProvider | |||
from pyup.providers.gitlab import Provider as GitlabProvider | |||
from pyup.providers.bitbucket_server import Provider as BitbucketServerProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Atlassian itself unified their branding, lets use just bitbucket
here. By the way, I assume this work for Their on-premise and cloud versions, right?
@@ -13,12 +14,12 @@ | |||
@click.command() | |||
@click.version_option(__version__, '-v', '--version') | |||
@click.option('--repo', prompt='repository', help='') | |||
@click.option('--user-token', prompt='user token', help='') | |||
@click.option('--user-token', prompt='user token', help='When using bitbucket_server, use this format: user@token@base_url') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In regards to the help string. I would try documenting this somewhere else and let this to contain either none or some generic short help string.
class Provider(object): | ||
name = "bitbucket_server" | ||
|
||
def __init__(self, bundle, intergration=False, url=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a typo here on intergration
?
Also, we have now an ignore-ssl
option. Is there a way to add that here?
def _api(self, token): | ||
""" | ||
Create a stashy connection object with the given token. | ||
:param token: should be in format: "user@token@base_url" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This format is really weird. Why do we need a base-url here anyway? I think you can rely on self.url
.
user = parts[0] | ||
token = parts[1] | ||
base_url = parts[2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user, token, base_url = parts
return stashy.connect(base_url, user, token) | ||
|
||
def get_user(self, token): | ||
# TODO: Return some kind of Bitbucket Server User object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't looks like our provider interface requires returning one specific kind of object here. So, it would be OK if Stashy has some user object, dict or something.
@@ -25,7 +25,10 @@ | |||
"python-gitlab>=1.3.0", | |||
"dparse>=0.4", | |||
"safety", | |||
"jinja2>=2.3" | |||
"jinja2>=2.3", | |||
"GitPython>=2.1.11", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are we using this, please?
"jinja2>=2.3", | ||
"GitPython>=2.1.11", | ||
"stashy", | ||
"requests-toolbelt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this dependency. It would be good to reduce dependencies as much as possible.
Do you think there are any other parts of our code that could benefit from using this?
self.provider = Provider(bundle=Mock()) | ||
self.provider._api = Mock() | ||
self.repo = Mock() | ||
self.token = "foo@foo@http://example.com/stash" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the base_url
looks weird. Can we make something like this instead?
provider = Provider(bundle=Mock(), url="http://example.com/stash")
token = "foo@bar"
What is the status of this pull request? What prevents it from being merged? |
any news on this? |
Hi! We are revamping our tools, so it isn't merged yet because we want to make sure PyUp CLI features play nicely and makes sense with the other tools and features. For instance, Safety 2.0 is in the process of becoming stable (https://github.com/pyupio/safety/releases). I'll take a look at this PR in the following weeks, and if it plays nicely with the plans for all our tools, I'll merge it. |
We (@timko98 and I) would like to propose an additional provider to pyup. The provider implements support for Atlassian Bitbucket Server by using Stashy with additional functionality by us.
Some of our solutions differ in the concepts from the Github and Gitlab implementations since the python client for the Bitbucket Server API was limited to Stashy which resulted in some workarounds. Even though this provider may not fully comply with existing providers this might be a basis for future work.
If you have any additional requests, questions or remarks, feel free to contact us.
Example usage
pyup --provider bitbucket_server --repo <projectname>/<reponame> --user-token <username>@<user_token>@<host>