Skip to content
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

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

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

Conversation

TobiasZeuch181
Copy link
Contributor

@TobiasZeuch181 TobiasZeuch181 commented Nov 8, 2024

… 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 loads the optional attributes enabled, autorefresh and gpgcheck and these values are used to fill up the repository-object that is later written to disk.
Unfortunately, these values are overriding the parameters, 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:

  1. set default values into
  2. override with existing values from file
  3. override with attributes from the ansible task configuration, if provided

Fixes #8783

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

zypper_repository

ADDITIONAL INFORMATION
  1. Register a repostory, e.g. via ansible
  2. use ansible to modify any of the parameters. The task will not change them

… 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
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module plugins plugin (any type) labels Nov 8, 2024
@felixfontein felixfontein changed the title Preserve attributes enabled, autorefresh and gpgcheck if provided from the… zypper_repository: preserve attributes enabled, autorefresh and gpgcheck if provided from the task parameters Nov 8, 2024
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch labels Nov 8, 2024
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I've added some first comments.

if module.params['autorefresh']:
repodata['autorefresh'] = '1'
else:
repodata['autorefresh'] = '0'
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

That depends on whether options have a default value or not. Having a default for options that are used for idempotency checks have this side-effect, which is usually not what the user wants. That's why it's best to only have default values for options that are not used for idempotency check, but never have defaults for options that are used for idempotency checks. (Unless there are very good reasons for a specific option; which can happen, but is not very likely.)

To fix this, it's usually best to remove the default values, and change the code so that if the user does not provide the option, the old default value is used when something is created.

Also please note that you are changing the behavior of the module, as I understand it, which is a breaking change. You need to control the behavior by a new option so that existing tasks will behave exactly as before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

I beg to differ, @felixfontein . If the value of the parameter is None, then that stmt is equivalent to if None:, which evaluates to False. He's not checking whether the parameter exists, he is using the parameter value, and None is... False-ish.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The statement if 'xxx' in module.params: is not equivalent to if None if module.params['xxx'] is None.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right of course. I was mislead by the fact that the lines of code selected for this comment do not include that part. Re-reading I see I missed some bits, and I have even submitted a suggestion fixing that up.

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
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Nov 18, 2024
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 ansibullbot added integration tests/integration needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR tests tests and removed stale_ci CI is older than 7 days, rerun before merging labels Nov 29, 2024
if module.params['autorefresh']:
repodata['autorefresh'] = '1'
else:
repodata['autorefresh'] = '0'
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

That depends on whether options have a default value or not. Having a default for options that are used for idempotency checks have this side-effect, which is usually not what the user wants. That's why it's best to only have default values for options that are not used for idempotency check, but never have defaults for options that are used for idempotency checks. (Unless there are very good reasons for a specific option; which can happen, but is not very likely.)

To fix this, it's usually best to remove the default values, and change the code so that if the user does not provide the option, the old default value is used when something is created.

Also please note that you are changing the behavior of the module, as I understand it, which is a breaking change. You need to control the behavior by a new option so that existing tasks will behave exactly as before.

@ansibullbot ansibullbot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Dec 5, 2024
@russoz
Copy link
Collaborator

russoz commented Dec 6, 2024

Other than the small detail in the changelog fragment, LGTM

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Dec 14, 2024
@felixfontein
Copy link
Collaborator

@TobiasZeuch181 ping

needs_info

@ansibullbot ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Dec 30, 2024
@ansibullbot
Copy link
Collaborator

@TobiasZeuch181 This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.

click here for bot help

@TobiasZeuch181
Copy link
Contributor Author

Sorry for the delay. I am slowly progressing with setting up my test environment for the improvement. It seems, that it still doesn't help to "simply" disable a repository that is already installed.
I linked the file into a different ansible-repository and simply call the module via molecule there. Debugging would be nice, not sure if Imanage to set that up though

@ansibullbot ansibullbot removed the needs_info This issue requires further information. Please answer any outstanding questions label Feb 5, 2025
and add the defaults again, which I had removed in the previous step, to maintain the behaviour according to the previous documentation
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed stale_ci CI is older than 7 days, rerun before merging labels Feb 5, 2025
…sk-in-zypper-module.yaml

Co-authored-by: Felix Fontein <[email protected]>
@ansibullbot ansibullbot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Feb 5, 2025
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch bug This issue/PR relates to a bug check-before-release PR will be looked at again shortly before release and merged if possible. integration tests/integration module module plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I can't deactivate a repository with module zypper_repository
4 participants