Skip to content

zypper_repository: preserve attributes enabled, autorefresh and gpgcheck if provided from the task parameters #9108

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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

zypper_repository: preserve attributes enabled, autorefresh and gpgcheck if provided from the task parameters #9108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2004d37
Don't override enabled, autorefresh and gpgcheck if provided from the…
TobiasZeuch181 Sep 3, 2024
ba8eca9
Adding code to set default values
TobiasZeuch181 Nov 8, 2024
a016d5b
Reordering how the boolean parameters are filled
TobiasZeuch181 Nov 14, 2024
71df6f4
Updating the content of the changelog file
TobiasZeuch181 Nov 29, 2024
b5431b4
Adding tests for modifying the repository
TobiasZeuch181 Nov 29, 2024
2635f51
Remove extra line that breaks the PEP8 sanity check
TobiasZeuch181 Dec 5, 2024
cefb03a
remove defaults and replace logic for checking defined parameters
TobiasZeuch181 Dec 6, 2024
48ad95c
Simplify the check for flags
TobiasZeuch181 Feb 5, 2025
aa29475
Update changelogs/fragments/8783-preserve-parameters-provided-from-ta…
TobiasZeuch181 Feb 5, 2025
0e43696
Use correct default-value for autorefresh
TobiasZeuch181 Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/8783-preserve-parameters-provided-from-task-in-zypper-module.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfixes:
- zypper-module - only read attributes ``enabled``, ``disable_gpg_check`` and ``autorefresh`` from local repo files, if the parameters were not already provided by the task parameters (https://github.com/ansible-collections/community.general/pull/9108).
27 changes: 14 additions & 13 deletions plugins/modules/zypper_repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,19 +356,6 @@ def main():
'name': module.params['description'],
'priority': module.params['priority'],
}
# rewrite bools in the language that zypper lr -x provides for easier comparison
if module.params['enabled']:
repodata['enabled'] = '1'
else:
repodata['enabled'] = '0'
if module.params['disable_gpg_check']:
repodata['gpgcheck'] = '0'
else:
repodata['gpgcheck'] = '1'
if module.params['autorefresh']:
repodata['autorefresh'] = '1'
else:
repodata['autorefresh'] = '0'

def exit_unchanged():
module.exit_json(changed=False, repodata=repodata, state=state)
Expand All @@ -393,6 +380,14 @@ def exit_unchanged():
if not alias and state == "present":
module.fail_json(msg='Name required when adding non-repo files.')

# fill boolean attributes with defaults
if 'enabled' not in repodata:
repodata['enabled'] = '0'
if 'autorefresh' not in repodata:
repodata['autorefresh'] = '0'
if 'gpgcheck' not in repodata:
repodata['gpgcheck'] = '0'

# Download / Open and parse .repo file to ensure idempotency
if repo and repo.endswith('.repo'):
if repo.startswith(('http://', 'https://')):
Expand Down Expand Up @@ -443,6 +438,12 @@ def exit_unchanged():
if 'gpgcheck' in repofile_items:
repodata['gpgcheck'] = repofile_items['gpgcheck']

# override boolean parameters in repodata with provided entries in module.params
# in the language that zypper lr -x provides for easier comparison
repodata['enabled'] = '1' if module.params['enabled'] else '0'
repodata['gpgcheck'] = '0' if module.params['disable_gpg_check'] else '1'
repodata['autorefresh'] = '1' if module.params['autorefresh'] else '0'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above three lines are indiscriminantly overwriting the values from the repo file (and the defaults set in the code above) with the values provided by the user (resp. the module defaults).

(Also this now overrides the values from the repo, while before the repo values overrode the values provided by the user. I guess both are wrong, but the current way is a breaking change and the old way has been the case for a longer time, so the module should stick to it unless the user explicitly tells the module to not do so.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Felix,

sorry, I don't get why do you say that the current implementation is probably wrong. Updating the value in the repo-file with the value provided by the user is the purpose of the module. And updating with the module-default is something that I can't prevent, unless I remove the default-value from the parameter-definition, right?

About this being a breaking change: I agree, but the current implementation makes it impossible to update any of these fields. Calling the module with an existing repo-path will succeed but have no impact on the repository configuration at all.
IF someone had such a task in his playbook that had no effect until now then the playbook would behave differently though with my change.

So what would you suggest here? You said before that I would need to introduce a new parameter to activate the new behaviour, would that be the way to go?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #9108 (comment). You need to remove the defaults and handle them in code.


exists, mod, old_repos = repo_exists(module, repodata, overwrite_multiple)

if alias:
Expand Down
38 changes: 38 additions & 0 deletions tests/integration/targets/zypper_repository/tasks/zypper_repository.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,44 @@
that:
- added_again_by_repo_file is not changed

- name: change the repository config by name with flag - disable
community.general.zypper_repository:
name: systemsmanagement_Uyuni_Stable
state: present
enabled: false
register: modified_repo_file_result

- name: modified_repo_file_result is changed
assert:
that:
- modified_repo_file_result is changed

- name: get repository details again from zypper after disabling the repository
command: zypper --xmlout lr systemsmanagement_Uyuni_Stable
register: get_repository_details_from_zypper

- name: verify modifying via .repo file was successful - the module is now disabled
assert:
that:
- "'enabled=\"0\"' in get_repository_details_from_zypper.stdout"

- name: change flag autorefresh in the same zypper-module
community.general.zypper_repository:
name: systemsmanagement_Uyuni_Stable
state: present
autorefresh: false
register: modified_repo_file_result

- name: get repository details again from zypper after disabling the repository
command: zypper --xmlout lr systemsmanagement_Uyuni_Stable
register: get_repository_details_from_zypper

- name: verify modifying via .repo file was successful - the autorefesh is disabled, but the enabled-flag was not modified (it is still disabled)
assert:
that:
- "'enabled=\"0\"' in get_repository_details_from_zypper.stdout"
- "'autorefresh=\"0\"' in get_repository_details_from_zypper.stdout"

- name: remove repository via url to .repo file
community.general.zypper_repository:
repo: http://download.opensuse.org/repositories/systemsmanagement:/Uyuni:/Stable/openSUSE_Leap_{{ ansible_distribution_version }}/systemsmanagement:Uyuni:Stable.repo
Expand Down