zypper_repository: preserve attributes enabled, autorefresh and gpgcheck if provided from the task parameters #9108
Conversation
… parameters Currently, the values are overridden with the data found on the file system Instead, these values should only be updated, if they were not provided in the parameters. That way, repositories on the system could be enabled/disabled and the flags could be updated again
if the values are not provided from the task-input nor read from an existing file
ansibullbot
added
bug
felixfontein
changed the title
felixfontein
added
check-before-release
plugins/modules/zypper_repository.py
Outdated
| if module.params['autorefresh']: | ||
| repodata['autorefresh'] = '1' | ||
| else: | ||
| repodata['autorefresh'] = '0' |
There was a problem hiding this comment.
module.params always contains all possible module options, so 'xxx' in module.params will always evaluate to True (if xxx is a valid module option). Also these three options have defaults, so even if the user did not provide them, they will always be set to their default value in module.params.
If you want to test whether the values have been provided by the user or not, you need to remove the defaults, and check for None instead (like if module.params['autorefresh'] is not None:).
There was a problem hiding this comment.
Interesting. I thought, I had tested it locally and adding the condition works. I'll give this another try and I'll also try to add some tests
There was a problem hiding this comment.
On the other hand I think this might break the logic where the plugin compares the configuration passed via module.params against what is found on the file system - not passing a value might result in the code detecting a change in configuration....
Also: is this the default behaviour in ansible-modules: not passing a parameter that has a default configured will change an existing configuration that was configured against the default value? That's something, I wouldn't expect
| @@ -0,0 +1,2 @@ | |||
| bugfixes: | |||
| - keycloak_client - fix TypeError when sanitizing the ``saml.signing.private.key`` attribute in the module's diff or state output. The ``sanitize_cr`` function expected a dict where in some cases a list might occur (https://github.com/ansible-collections/community.general/pull/8403). | |||
There was a problem hiding this comment.
This message is about another module and doesn't make sense for this one.
There was a problem hiding this comment.
Right, sorry, I had edited the changes but not saved them...
I think, this is the right approach: # fill the boolean entries with default # override the values with what is found in the repo-file (remote or locally on the file) # finally override the values with what is provided in the module-params then we can check if the data has changed compared to the local data
I had copied and edited the data but not saved the changes
adding tests: * modifying a repository should work (disabling) * modifying a different parameter doesn't change configuation that was not specified - although it has a different default-value, than what is currenctly configured
ansibullbot
added
integration
… task parameters
Currently, the values are overridden with the data found on the file system Instead, these values should only be updated, if they were not provided in the parameters. That way, repositories on the system could be enabled/disabled and the flags could be updated again
SUMMARY
The code currently looks up the repo-file on the local disk and if it exists, it load the optional attributes
enabled,autorefreshandgpgcheckand these values are used to fill up the repository-object that is later written to disk.Unfortunately, these values are overriding the attributes, if they were provided from the task in ansible - which makes it impossible to modify the values for a repository that is already registered on the system.
So I changed the code, to:
I considered changing the order of the statements to
but I thought it was better to stay with the current structure, because the change is smaller and we need the code to read the ansible-parameters at the beginning to check, if there parameters would introduce a change.
Fixes 8783
ISSUE TYPE
COMPONENT NAME
zupper_repository
ADDITIONAL INFORMATION