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

Module zypper_repository now does not fail when the repo already exists #45489

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@pdostal
Copy link

pdostal commented Sep 11, 2018

SUMMARY

Friend of mine discovered an issue in the zypper_repository os packaging module:
When we want a repository defined in .repo file the module crashes if the repo already exists.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

zypper_repository

ANSIBLE VERSION
ansible 2.6.4
  config file = None
  configured module search path = ['/root/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.6/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.6.5 (default, Mar 31 2018, 19:45:04) [GCC]

ADDITIONAL INFORMATION

When you add a repository then everything works:

- zypper_repository:
    repo: 'http://download.opensuse.org/test/'
    name: 'test'
    state: present

But when you add a repository from .repo and the repo already exists then it fails:

- zypper_repository:
    repo: 'http://download.opensuse.org/test/test.repo'
    state: present

This is because we don't know the name of the repository perhaps we don't check the .repo file.
Now I check the repo file and I also compare that the name is the same and if it is we are done.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Sep 11, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/packaging/os/zypper_repository.py:338:59: E226 missing whitespace around arithmetic operator

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Sep 11, 2018

@@ -330,6 +333,10 @@ def main():
'name': module.params['description'],
'priority': module.params['priority'],
}
# Get name from .repo file
if repodata['url'].endswith('.repo'):
rc, stdout, stderr = module.run_command('curl -s '+repodata['url'], check_rc=False)

This comment has been minimized.

@webknjaz

webknjaz Sep 11, 2018

Member

maybe just analyze what failing zypper command returns instead of downloading this twice?

@pdostal pdostal force-pushed the pdostal:zypper_repository branch 5 times, most recently Sep 12, 2018

@pdostal

This comment has been minimized.

Copy link
Author

pdostal commented Sep 12, 2018

OK, so now I use zypper repository list to find the name of given repository.

  • If the repository doesn't exist, zypper will fetch it while adding.
  • If the repository does exist, we know the name so we can check it.

I know there's still additional command but now it requires no additional request.

lib/ansible/modules/packaging/os/zypper_repository.py Outdated
@@ -183,7 +184,8 @@ def _repo_changes(realrepo, repocmp):
if k == "url":
valold, valnew = valold.rstrip("/"), valnew.rstrip("/")
if valold != valnew:

This comment has been minimized.

@webknjaz

webknjaz Sep 12, 2018

Member

Instead of nesting, use valold != valnew and realrepo['name'] != repocmp['name'] clause here.

lib/ansible/modules/packaging/os/zypper_repository.py Outdated
if repodata['url'] and repodata['url'].endswith('.repo'):
bareurl = repodata['url'].rsplit('/', 1)[0]
rc, stdout, stderr = module.run_command("sh -c 'zypper lr -u | grep " + bareurl + " | cut -d\"|\" -f3'", check_rc=False)
if isinstance(stdout, str) and stdout.strip():

This comment has been minimized.

@webknjaz

webknjaz Sep 12, 2018

Member

should probably use if stdout is not None:

Module zypper_repository now does not fail when the repo already exists
Module zypper_repository now does not fail when the repo already exists

Do not use curl in zypper_repository module

@pdostal pdostal force-pushed the pdostal:zypper_repository branch to d2a19fb Sep 12, 2018

@ansibot ansibot added the stale_ci label Sep 20, 2018

@pdostal

This comment has been minimized.

Copy link
Author

pdostal commented Oct 8, 2018

Hello, @webknjaz, what is the status of this PR?

@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Oct 8, 2018

@pdostal hi, it's waiting for approvals from maintainers @matze @robinro. It's not core-supported module, so we don't maintain it, nor know about specifics of how it's supposed to work.

bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Oct 8, 2018

Components

lib/ansible/modules/packaging/os/zypper_repository.py
support: community
maintainers: matze robinro

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 0
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainer or core team member): []
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

@@ -182,7 +183,7 @@ def _repo_changes(realrepo, repocmp):
valnew = v or ""
if k == "url":
valold, valnew = valold.rstrip("/"), valnew.rstrip("/")
if valold != valnew:
if valold != valnew and realrepo['name'] != repocmp['name']:

This comment has been minimized.

@robinro

robinro Oct 11, 2018

Contributor

I don't understand this change.

This comment has been minimized.

@pdostal

pdostal Oct 12, 2018

Author

I check also name - not just URL.

# Get name from .repo file
if repodata['url'] and repodata['url'].endswith('.repo'):
bareurl = repodata['url'].rsplit('/', 1)[0]
rc, stdout, stderr = module.run_command("sh -c 'zypper lr -u | grep " + bareurl + " | cut -d\"|\" -f3'", check_rc=False)

This comment has been minimized.

@robinro

robinro Oct 11, 2018

Contributor

Parsing zypper output this way is very brittle and guranteed to fail on upgrades.
Also: What if the content of the .repo file changes and the name differs?

This comment has been minimized.

@pdostal

pdostal Oct 12, 2018

Author

I parsed it using curl directly from the .repo file but that was unwanted HTTP request... What do you @robinro suggest?

This comment has been minimized.

@robinro

robinro Oct 17, 2018

Contributor

I'm not sure what the best solution is, but at the moment I would lean towards not supporting .repo files and asking people to specify the repo URL directly. To properly support .repo files we would have to download them, parse them and compare them to the configured repos, which is a lot of effort.

Are you aware of a usecase that actually requires .repo files? I no longer use suse in my day job and all repo usage that I can remember also worked fine with pointing to directories directly.

@ansibot ansibot added the packaging label Feb 17, 2019

@gundalow gundalow added the zypper label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.