-
Notifications
You must be signed in to change notification settings - Fork 42
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 href support to resource_option #343
Conversation
6b0818a
to
bd897a6
Compare
This is currently only implemented in "pulp file repository *" commands to showcase. |
e80b7ce
to
ff2a721
Compare
pulp_href: Optional[str] = None | ||
entity: Optional[EntityDefinition] = None | ||
|
||
if value.startswith("/"): |
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 wonder if this should check for href_pattern
instead and then just assume it's a name if it doesn't match:
$ pulp file remote create --name /test/ --url https://fixtures.pulpproject.org/file/
$ pulp file repository create --name /test/ --remote /test/
Error: '/test/' is not a valid href for remote.
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 wouldn't solve the problem where the name actually looks like a valid href.
But you can always use: $ pulp file repository create --name /test/ --remote ::/test/
ff2a721
to
1cb5de5
Compare
[noissue]
ec3689e
to
736accb
Compare
@daviddavis I extended the use of this feature to all plugins sync and export{,er}s. You might want to revisit. |
I had one thought (not necessarily for this PR): what if we stored the href_pattern on the context instead of having to define it everywhere? |
I'm happy to move it around (to generic?). But for the more generic options, what should be the Context to store it on? |
Maybe for generic options, the href_pattern must be supplied when calling resource_option? |
I moved it into the base context. It seemd to match best there. |
736accb
to
65f1290
Compare
pulpcore/cli/core/export.py
Outdated
@@ -16,16 +18,34 @@ | |||
from pulpcore.cli.core.context import PulpExportContext, PulpExporterContext | |||
|
|||
_ = gettext.gettext | |||
REPO_HREF_PATTERN = r"^/pulp/api/v3/repositories/(?P<plugin>\w+)/(?P<resource_type>\w+)/" |
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.
Why define this here instead of using PulpRepositoryContext.HREF_PATTERN
?
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.
forgot about that.
I was thinking that if href_pattern is None and hasattr(ctx, HREF_PATTERN):
href_pattern = ctx.HREF_PATTERN |
The problem is, it needs the pattern before determining the Context class to instanciate. |
65f1290
to
8182d69
Compare
Ah, I see. That kind of defeats part of the purpose of my suggestion to have it on context then. |
It still feels like the right place. |
fixes #315