-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat(rpm_repository): add retain_repo_versions parameter #171
Conversation
This is related to #165 |
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.
How do you feel about adding tests?
These modules are about to be transfered to a new framework. Adding this parameter to the tests will ensure it's not immediately broken afterwards.
(Please do not test the retain version feature, but just that the model is able to set it on a repository.)
plugins/modules/rpm_repository.py
Outdated
@@ -41,6 +41,10 @@ | |||
- Max number of latest versions of each package to keep (optional) | |||
type: int | |||
version_added: "0.0.16" | |||
retain_repo_versions: | |||
description: | |||
- Max number of repository versions to keep (optional) |
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.
- Max number of repository versions to keep (optional) | |
- Max number of repository versions to keep |
not being required is implied by ansible. Can you adjust line 41 too?
Also version_added may be nice.
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.
Thanks for the feedback, @mdellweg . I've made the adjustments and will submit them soon.
Can you confirm what you would like to see regarding tests? I have produced 2 tests:
- set
retain_repo_versions
to 5 - update
retain_repo_versions
to 1 (i.e. don't keep old versions)
I have ran and recorded the tests--they all pass. However, I see the following now:
- all existing
tests/fixtures/rpm_repository-*.yml
files have been modified - 2 new fixture files were created:
test/fixtures/rpm_repository-{22,23}.yml
What portions of the tests/results would you like committed?
Side note:
There seems to be a limitation in which every call that includes the retain_repo_versions
parameter updates the returned pulp_last_updated
timestamp (no other returned values differ). This is the case even when retain_repo_versions
is not changed from the previous call. I assume this is a bug in pulpcore
itself; however, it does limit the ability to assert results.changed == false
on repeated calls with the same value for retain_repo_versions
.
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 portions of the tests/results would you like committed?
All of them. They are separate files for technical reasons only.
Side note: [...]
That actually sounds like a bug on the client side. When we don't want to change the value, we must not do a patch call, and that still seems to happen.
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.
Hey @mdellweg, I have changed the tests to try for idempotence and am not seeing the previous issue. I believe that I must have made an error in my original work. Please see my changes and let me know if there is anything else needed for approval.
I appreciate you allowing me to contribute to this project.
07678c0
to
74b2877
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.
Thank you!
Looks like you need to run "make format" once. |
This change adds `retain_repo_versions` parameter option to `rpm_repository` module. It also adds tests and associated fixtures. fixes pulp#165
Hey @mdellweg, I've pushed the newly formatted code. |
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.
Thank you for following along.
This change adds
retain_repo_versions
parameter option torpm_repository
module.