Add retained_versions option to repository commands. #210
Conversation
daviddavis
force-pushed
the
210
branch
2 times, most recently
from
125f4f0 to
ec84c7c
Compare
| def __init__( | ||
| self, | ||
| *args: Any, | ||
| needs_plugin: Optional[str] = None, |
There was a problem hiding this comment.
Should we allow a single version to require multiple plugins?
May be overkill...
There was a problem hiding this comment.
I considered this and thought it was unlikely that an option would require more than one plugin. It's possible that an option would require a specific plugin version and a core version but then I'd expect the plugin to pin that version of core.
There was a problem hiding this comment.
I accept that argument. We can still consider to attach this variable directly to the PluginRequiredVersion tuple.
It would even spare us some validation.
There was a problem hiding this comment.
So devs would supply a PluginRequiredVersion when setting needs_plugin? The only validation that this would save us from is having to check that needs_plugin is set if min/max version is supplied. The trade off is that we have to import and instantiate PluginRequiredVersion every time we want to set the option requirement. I'm not opposed to it but it doesn't seem like a decisive advantage.
There was a problem hiding this comment.
Here's a PoC for having a needs_plugins parameter that takes PluginRequiredVersion (which I've renamed to PluginRequirement due to fewer characters). Let me know what you think. If you think it's better, I can merge it into this PR.
There was a problem hiding this comment.
I like the name PluginRequirement.
Need to think about the whole change. I thought, it would also change the signature of needs_plugin and has_plugin, but that might be a bad idea at this point in time.
Maybe we should think about this separately from this pr.
There was a problem hiding this comment.
Ok, sounds good. I'll rename PluginRequiredVersion to PluginRequirement here and then open a new PR for us to consider.
| pulp_ctx.needs_plugin( | ||
| self.needs_plugin, | ||
| self.min_version, | ||
| self.max_version, | ||
| _("the {name} option").format(name=self.name), | ||
| ) |
daviddavis
force-pushed
the
210
branch
3 times, most recently
from
0a76e5f to
c7ea688
Compare
pulpcore/cli/common/context.py
Outdated
| @@ -33,6 +33,14 @@ | |||
| RepositoryDefinition = Tuple[str, str] # name, pulp_type | |||
| RepositoryVersionDefinition = Tuple[str, str, int] # name, pulp_type, version | |||
|
|
|||
|
|
|||
| class PluginRequiredVersion(NamedTuple): | |||
There was a problem hiding this comment.
Had to move this here to avoid circular dependency.
|
This is going to fail until pulp/pulpcore#1204 is merged. |
pulpcore/cli/ansible/distribution.py
Outdated
| from pulpcore.cli.common.context import ( | ||
| EntityDefinition, | ||
| EntityFieldDefinition, | ||
| PulpContext, | ||
| pass_entity_context, | ||
| pass_pulp_context, | ||
| ) | ||
| from pulpcore.cli.common.generic import PluginRequiredVersion as PRV | ||
| from pulpcore.cli.common.context import EntityDefinition, EntityFieldDefinition | ||
| from pulpcore.cli.common.context import PluginRequiredVersion as PRV | ||
| from pulpcore.cli.common.context import PulpContext, pass_entity_context, pass_pulp_context |
There was a problem hiding this comment.
What was bad about the old version of bunched import?
There was a problem hiding this comment.
Isort automatically did this. It breaks alias imports (PluginRequiredVersion as PRV) into a separate line. Since PluginRequiredVersion moved from pulpcore.cli.common.generic to pulpcore.cli.common.context, it broke up the pulpcore.cli.common.context into multiple lines you see here to keep the alphabetic order.
pulpcore/cli/common/context.py
Outdated
| @@ -74,7 +82,7 @@ class PulpContext: | |||
| def __init__(self, api_kwargs: Dict[str, Any], format: str, background_tasks: bool) -> None: | |||
| self._api: Optional[OpenAPI] = None | |||
| self._api_kwargs = api_kwargs | |||
| self._needed_plugins: List[Tuple[str, Optional[str], Optional[str]]] = [] | |||
| self._needed_plugins: List[PluginRequiredVersion] = [] | |||
daviddavis
force-pushed
the
210
branch
4 times, most recently
from
f438e3b to
f3a2119
Compare
[noissue]