-
Notifications
You must be signed in to change notification settings - Fork 170
Namespace plugin endpoints with Django app label #3836
Conversation
Hello @goosemania! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on January 23, 2019 at 21:05 Hours UTC |
@dralley , I'd appreciate your critical look at this PR. It results in the following (notice
|
I'll fix tests and plugins after LGTM on approach here. |
@goosemania Approach looks good, LGTM |
Codecov Report
@@ Coverage Diff @@
## master #3836 +/- ##
=========================================
+ Coverage 55.57% 55.6% +0.03%
=========================================
Files 66 66
Lines 2818 2820 +2
=========================================
+ Hits 1566 1568 +2
Misses 1252 1252
Continue to review full report at Codecov.
|
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.
Had a question. Other than that LGTM.
|
||
FILE_REMOTE_PATH = urljoin(BASE_REMOTE_PATH, 'file/') | ||
FILE_CONTENT_PATH = urljoin(CONTENT_PATH, 'files/') |
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'm not sure I understand this change. Shouldn't FILE_CONTENT_PATH
still be /pulp/api/v3/content/file/files/
and not /pulp/api/v3/content/files/
?
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.
@daviddavis , you're right. Thank you!
I was surprised, why tests didn't fail, so after some investigation, I found a problem with installation of pulp_file, so test were skipped. Fixed as well now
9c86366
to
d65575a
Compare
closes #4279 https://pulp.plan.io/issues/4279 Required PR: pulp/pulp_file#157
@daviddavis , ready for re-review |
re #4279 https://pulp.plan.io/issues/4279 Required PR: pulp/pulp#3836
re #4279 https://pulp.plan.io/issues/4279 Required PR: pulp/pulp#3836
This looks good to me. Thanks for putting this together! |
The PR pulp/pulp#3836 removed the `pulp_` prefix from app labels. So commands like `pulp-manager makemigrations pulp_file` are now `pulp-manager makemigrations file`
The PR pulp/pulp#3836 removed the `pulp_` prefix from app labels. So commands like `pulp-manager makemigrations pulp_file` are now `pulp-manager makemigrations file`
re #4279 https://pulp.plan.io/issues/4279 Required PR: pulp/pulp#3836
closes #4279
https://pulp.plan.io/issues/4279