-
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 support for new pulp_rpm "prune-packages" feature. #977
Conversation
Needed:
|
b2c4afc
to
c1ee4b1
Compare
pulpcore/cli/rpm/prune.py
Outdated
if all_repositories: | ||
repo_hrefs = ["*"] | ||
else: | ||
repo_hrefs = [ | ||
repository_ctx.pulp_href | ||
for repository_ctx in repositories | ||
if isinstance(repository_ctx, PulpRpmRepositoryContext) | ||
] | ||
|
||
params = { | ||
"repo_hrefs": repo_hrefs, | ||
"keep_days": keep_days, | ||
"dry_run": dry_run, | ||
} |
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.
Please try this:
if all_repositories: | |
repo_hrefs = ["*"] | |
else: | |
repo_hrefs = [ | |
repository_ctx.pulp_href | |
for repository_ctx in repositories | |
if isinstance(repository_ctx, PulpRpmRepositoryContext) | |
] | |
params = { | |
"repo_hrefs": repo_hrefs, | |
"keep_days": keep_days, | |
"dry_run": dry_run, | |
} | |
if all_repositories: | |
repositories = ["*"] | |
params = { | |
"repo_hrefs": repositories, | |
"keep_days": keep_days, | |
"dry_run": dry_run, | |
} |
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 had tried that and it failed with "'repo_hrefs' is expected to be a list." I just did a little deeper experimentation,. and discovered that the problem is that, coming into this call, repositories is a generator. If I list(repositories), the correct validation happens in the correct place and All Is Well :) next update will address.
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.
Nope - you can't "just pass" the generator, and you can't pass list(repositories)
because mypy complains error: List comprehension has incompatible type List[str | PulpEntityContext | None]; expected List[str]
and the lint-check fails.
Restoring to original comprehension
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.
Hmm. This raises the question whether it's good to allow ["*"]
as a "list of hrefs" on the server side. How is this represented in the openapi spec? If it were a "list of repository_hrefs" i'd expect to see {"type": "array", "items": {"type": "str", "format": "uri"}
.
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.
What happens if you provided an empty list?
What, if you don't pass it at all?
The output of pulp debug openapi operation --id rpm_prune_prune
or so should help us. The referenced (body) schema is of interest too.
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.
What happens if you provided an empty list? What, if you don't pass it at all?
$ pulp rpm prune-packages
Error: at least one --repository, or --all-repositories, must be specified
The output of
pulp debug openapi operation --id rpm_prune_prune
or so should help us. The referenced (body) schema is of interest too.
$ pulp debug openapi schema --name PrunePackages
{
"type": "object",
"description": "Serializer for prune-old-Packages operation.",
"properties": {
"repo_hrefs": {
"type": "array",
"items": {
"type": "string",
"minLength": 1
},
"description": "Will prune old packages from the specified list of repos. Use ['*'] to specify all repos. Will prune based on the specified repositories' latest_versions."
},
"keep_days": {
"type": "integer",
"format": "int64",
"default": 14,
"description": "Prune packages introduced *prior-to* this many days ago. Default is 14. A value of 0 implies 'keep latest package only.'",
"minimum": 0
},
"dry_run": {
"type": "boolean",
"default": false,
"description": "Determine what would-be-pruned and log the list of packages. Intended as a debugging aid."
}
},
"required": [
"repo_hrefs"
]
}
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.
So repo_hrefs is required and must contain at least one href or "*".
Thanks, that gives some insight.
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.
See current refactoring - took some pointers from #749 , now simpler and keeps mypy happy.
99fa27a
to
9010bed
Compare
No description provided.