-
Notifications
You must be signed in to change notification settings - Fork 107
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
Move pulpcore-plugin into pulpcore as a subpackage #347
Conversation
fcebe6a
to
98294ad
Compare
3d272d0
to
7b01dbd
Compare
7b01dbd
to
926428f
Compare
@@ -0,0 +1,364 @@ | |||
import asyncio |
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 think these test
_files are in the code base itself, but I expected them in
pulpcore/tests` https://github.com/pulp/pulpcore/tree/master/pulpcore/tests There should likely be both unit and functional tests. Also I checked Travis and they are not running there. The Travis tests need to pass with these the pulpcore-plugin tests also.
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.
Fixed
768e1f9
to
b044dc6
Compare
pulpcore/plugin/__init__.py
Outdated
@@ -0,0 +1,7 @@ | |||
__version__ = '0.1.0rc8.dev' |
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 was thinking we will not have another version number for this package. We had talked about doing that, but since we couldn't depend on it with setuptools, we can just use pulpcore
. For example with the changelog, we'll add custom towncrier extensions which will build two "sections" of the changelog, one for users, one for plugin writers. Both would use the version of pulpcore itself though (not this one). @daviddavis @dralley @dkliban does this sound right?
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 is such a big PR though I'm in favor of merging as-is and fixing in a separate PR.
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.
That sounds right to be based on my memory of our discussion.
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 is exactly as I remember it.
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.
Line deleted
Let's remove this later, if possible. [noissue]
b044dc6
to
f499db6
Compare
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 looks good to me. Let's merge and we can make additional improvements afterwards. Thanks @dralley ! 💯
Required PR: pulp/pulpcore#347 re: #5580 ihttps://pulp.plan.io/issues/5580
Required PR: pulp/pulpcore#347 re: #5580 ihttps://pulp.plan.io/issues/5580
re: #5580
https://pulp.plan.io/issues/5580