-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add opendev.org https and git fetcher #553
Conversation
60d8387
to
50af1ca
Compare
50af1ca
to
2cbbc17
Compare
Ready for review. Just to confirm, I did a rebuild of the previously offending charm, and the build succeeds with this branch of charmtools.
|
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.
The change looks good.
Out of scope of this change, the classes look like they are calling out for some refactoring and inheritance to avoid boilerplate duplicate code.
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.
There's a lot of duplicated implementation between GitFetcher
, GitHubFetcher
, LaunchpadGitFetcher
, and OpendevFetcher
. I think it would be good to refactor this so that they all subclass from GitFetcher
and extend as needed to reduce duplicated code. (For example, the GitFetcher
is currently missing the revision handling that was added to the others.)
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.
My previous comment was not intended as a nack, just to call out that this section of code could really use some cleaning up at some point. PR is fine as-is for the immediate fix.
2.7.2 has been released to PyPI with (only) this change. |
@johnsca 😸 Thanks! |
Related to #552
https://bugs.launchpad.net/charm-barbican/+bug/1846263