Call zypper refresh after adding/modifying a repository

[dincamihai]

What does this PR do?

completes this previous PR #33432
and it attempts to fix this issue #27155

What issues does this PR fix or reference?

In my previous attempt to fix this I was tricked by zypper's caching mechanism which saves the gpg key once accepted and it made me believe that just fixing the --gpg-auto-import-keys position fixes the issue.
I talked today with the maintainer of 'zypper' and it seems that an additional call like this

zypper --gpg-auto-import-keys refresh

is necessary after adding the repository.
At the moment, this is the only way to prevent subsequent zypper refresh from asking for confirmation of the gpg key.
I didn't revert the previous commit introduced with #33432 because it still fixes the related issue with using --gpg-auto-import-keys in the wrong position when calling zypper.

Tests written?

Yes.
Meanwhile I learned more about zypper and modules/zypper.py::mod_repo so I added more tests to capture the behavior of mod_repo when passing different parameters to it and to make sure the repository is refreshed only when specifically requested in the sls file with gpgautoimport: True

Possible scenarios:

• url only

  repo_with_gpg:
    pkgrepo.managed:
      - url: {{ REPO_URL }}

  • when the repository is new:
    • this only adds the new repository and does not call modify nor refresh
  • when the repository already exist:
    • raises CommandExecutionError: Specified arguments did not result in modification of repo

• url and refresh

  repo_with_gpg:
    pkgrepo.managed:
      - url: {{ REPO_URL }}
      - refresh: True,

  • when the repository is new:
    • this adds the repository, then modifies it by updating its autorefresh option but doesn't call zypper refresh
  • when the repository already exist:
    • modifies the repository by updating its autorefresh option but doesn't call zypper refresh

• url and gpgautoimport

  repo_with_gpg:
    pkgrepo.managed:
      - url: {{ REPO_URL }}
      - gpgautoimport: True

  • when the repository is new:
    • this adds the repository and refreshes with zypper --gpg-auto-import-keys refresh <repo-name> but does not call modify
  • when the repository already exist:
    • this only calls zypper --gpg-auto-import-keys refresh <repo-name> on the existing repository

• url, refresh and gpgautoimport

  repo_with_gpg:
    pkgrepo.managed:
      - url: {{ REPO_URL }}
      - refresh: True,
      - gpgautoimport: True

  • when the repository is new:
    • this adds the repository, then modifies it by updating its autorefresh option and then refreshes with zypper --gpg-auto-import-keys refresh <repo-name>
  • when the repository already exist:
    • modifies the repository by updating its autorefresh option and calls refresh

[dincamihai]

Please do not review this yet because I just noticed that I need to add more things.

[dincamihai]

I think it's ready for review now (if all the tests pass, of course).

@isbm maybe you want to take a look as well

[cachedout]

@isbm Please let me know once you've had a chance to look this over and then I'll review it. Thanks.

[isbm]

I would not forge such deranged shatter with unnecessary trigger_refresh, but would just:

if kwargs.get('gpgautoimport'):
    if added:
        refresh_opts = global_cmd_opt ...
else:
    if not added:
        raise CommandExecutionError( ....

That would look simple and clear.

[dincamihai]

@isbm

if kwargs.get('gpgautoimport'):

is wrong because we don't want this:

repo_with_gpg:
  pkgrepo.managed:
    - url: http://some.url/path
    - gpgautoimport: any-string-here

[dincamihai]

@isbm
I do not like the idea of dropping the specialized variable because I want to have a single source of truth.
So I want to check only once this (I don't want to repeat this check again):

if kwargs.get('gpgautoimport') is True:

So I'm relying on that single line for both adding --gpg-auto-import-keys option to global_cmd_opt and for calling the refresh and not on two separate conditions.
If we later want to modify the condition, we change it in one single place.

[kbaikov]

Tested the patch and it worked with - gpgautoimport: True, but did not work with - gpgautoimport: 1.
I believe True and 1 should be accepted in salt states? Correct me if i am wrong.

[dincamihai]

@kbaikov I'm not aware of this, so if anyone know anything please speak up.
But, in case we also allow "1", I assume we wouldn't want allow - gpgautoimport: somestring because maybe someone will try - gpgautoimport: false (this translates to False so it's ok)

[isbm]

@kbaikov It won't work with "1/0", because the code is waiting only for True/False values.

[dincamihai]

@isbm we know that. the question is if 1 should work as True

[kbaikov]

I was under the impression that all states that accept True will work with 1 also. At least in some other states.

[isbm]

@kbaikov This is valid to Zypper configs. But this is not a Zypper config.

[isbm]

@dincamihai Where do you see "1/0" as "True/False" in the link I've sent? Maybe I am missing something.

[dincamihai]

@isbm the only thing that prevents gpgautoimport: 1 from working is this line https://github.com/saltstack/salt/blob/develop/salt/modules/zypper.py#L787
So we can change it to accept 1.
Do we want that?

[isbm]

@dincamihai No, we don't. Let's make it simple, true/false and not start implementing all possible ways of defining dis- allowance options.

[dincamihai]

@isbm I agree

[isbm]

@cachedout 👍 😉

[dincamihai]

It seems the computer shutdown while jenkins was running:

12:30:57 Broadcast message from root@ubuntu
12:30:57    (unknown) at 12:30 ...
12:30:57 
12:30:57 
The system is going down for power off NOW!
12:30:57 [CPU:33.3%|MEM:14.7%|Z:0]  ... OK (1.059s)
12:30:57   test_disable_job (unit.utils.schedule_test.ScheduleTestCase)
12:30:58 Connection to 45.33.53.215 closed by remote host.
12:30:58 Connection to 45.33.53.215 closed.

Is there a way to trigger a new build?

[cachedout]

Go Go Jenkins!