yum module properly check for None config_file #46641

maxamillion · 2018-10-08T19:23:01Z

Signed-off-by: Adam Miller admiller@redhat.com

SUMMARY

Fixes #46485
Fixes #46603

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

yum

ANSIBLE VERSION

ansible 2.8.0.dev0 (bugfix/46603-yum-config-empty c815343346) last updated 2018/10/08 14:20:35 (GMT -500)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/admiller/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/admiller/src/dev/ansible/lib/ansible
  executable location = /home/admiller/src/dev/ansible/bin/ansible
  python version = 2.7.15 (default, Sep 21 2018, 23:26:48) [GCC 8.1.1 20180712 (Red Hat 8.1.1-5)]

ansibot · 2018-10-08T19:58:50Z

@maxamillion: thank you for submitting this pull-request !

cc @Akasurde @berenddeschouwer @kustodian @verm666
click here for bot help

tamsky · 2018-10-08T23:34:04Z

@maxamillion : thanks for finding and correcting this.

Per your comment in #46603 (comment):

we have a lot of integration tests that perform simple installs/updates that all work as well

There are currently zero tests that exercise the conf_file param in:
https://github.com/ansible/ansible/blob/devel/test/integration/targets/yum/tasks/yum.yml

Would it be appropriate to also add an integration test that demonstrates failure before this PR, and success after this PR?

tamsky · 2018-10-09T00:30:19Z

I'm thinking something similar to the test you've committed at
https://github.com/ansible/ansible/pull/46634/files#diff-c772aa4d180b01664d6d840008c03440
would be good.

mkrizek · 2018-10-09T06:56:48Z

lib/ansible/modules/packaging/os/yum.py

@@ -1306,7 +1306,7 @@ def ensure(self, repoq):
        if self.conf_file and os.path.exists(self.conf_file):
            self.yum_basecmd += ['-c', self.conf_file]

-        if repoq:
+        if repoq and self.conf_file and os.path.exists(self.conf_file):


Just a nitpick but the same conditional self.conf_file and os.path.exists(self.conf_file) is a couple of lines above, why not just indent the original conditional if repoq there:

if self.conf_file and os.path.exists(self.conf_file): self.yum_basecmd += ['-c', self.conf_file] if repoq: repoq += ['-c', self.conf_file]

I honestly can't believe I didn't think of that. Will update, thanks.

Signed-off-by: Adam Miller <admiller@redhat.com>

maxamillion · 2018-10-09T20:55:12Z

@tamsky you make a good point and I'll definitely add an integration test but I thought it was weird that you were running into that issue without passing a config file (as per the example you provided) and passing a config file did in fact work around the issue so our tests wouldn't have caught it anyways.

tamsky · 2018-10-09T21:02:53Z

test/integration/targets/yum/tasks/yum.yml

@@ -135,6 +145,15 @@
    that:
        - "not yum_result is changed"

+- name: install sos with state latest idempotence with config file param
+  yum: name=sos state=latest
+  register: yum_result


is
config_file: /etc/yum.conf
missing from this test?

@tamsky it shouldn't be since that's the default one in the container instance as it executes in shippable, I'm just passing it to exercise that arg passing but I realized that because of a change in Ansible 2.7 I need to conditionalize it.

@tamsky I'm a doof, it absolutely is. Sorry, I misunderstood.

There remains an apparent mismatch between:
this test's name:
name: install sos with state latest idempotence with config file param
which includes the string "with config file param",

vs

this test's content, which lacks a config_file param
yum: name=sos state=latest

In the previous test being added, in this PR, back at L113... the test name matches the test content:

- name: install sos with state latest in check mode with config file param yum: name=sos state=latest conf_file=/etc/yum.conf

Would it be appropriate to update the second test's name: (on line 147) so that the test content matches the test name?
Or... am I confused about all this?

Any fixes, please backport to stable-2.7 as well.
Thanks.

mattclay · 2018-10-09T21:26:51Z

CI failure in integration tests: https://app.shippable.com/github/ansible/ansible/runs/87492/37/console

Signed-off-by: Adam Miller <admiller@redhat.com>

jctanner · 2018-10-11T13:03:37Z

@maxamillion please backport this to 2.7 once merged.

maxamillion · 2018-10-11T13:35:16Z

rebuild_merge

maxamillion · 2018-10-11T14:57:06Z

bot_status

* yum module properly check for None config_file * add conf_file test cases to yum integration tests Signed-off-by: Adam Miller <admiller@redhat.com> (cherry picked from commit fb6e91b)

* yum module properly check for None config_file (#46641) * yum module properly check for None config_file * add conf_file test cases to yum integration tests Signed-off-by: Adam Miller <admiller@redhat.com> (cherry picked from commit fb6e91b) * add changelog for 2.7 backport Signed-off-by: Adam Miller <admiller@redhat.com>

tamsky

@maxamillion
Sorry, I'm still confused.

tamsky · 2018-10-12T06:21:34Z

test/integration/targets/yum/tasks/yum.yml

@@ -135,6 +145,15 @@
    that:
        - "not yum_result is changed"

+- name: install sos with state latest idempotence with config file param
+  yum: name=sos state=latest
+  register: yum_result


There remains an apparent mismatch between:
this test's name:
name: install sos with state latest idempotence with config file param
which includes the string "with config file param",

vs

this test's content, which lacks a config_file param
yum: name=sos state=latest

In the previous test being added, in this PR, back at L113... the test name matches the test content:

- name: install sos with state latest in check mode with config file param yum: name=sos state=latest conf_file=/etc/yum.conf

Would it be appropriate to update the second test's name: (on line 147) so that the test content matches the test name?
Or... am I confused about all this?

Any fixes, please backport to stable-2.7 as well.
Thanks.

* yum module properly check for None config_file * add conf_file test cases to yum integration tests Signed-off-by: Adam Miller <admiller@redhat.com>

Akasurde removed the needs_triage Needs a first human triage before being processed. label Oct 9, 2018

mkrizek reviewed Oct 9, 2018

View reviewed changes

mkrizek approved these changes Oct 9, 2018

View reviewed changes

yum module properly check for None config_file

10ac23e

Signed-off-by: Adam Miller <admiller@redhat.com>

maxamillion force-pushed the bugfix/46603-yum-config-empty branch from c815343 to 10ac23e Compare October 9, 2018 20:53

tamsky reviewed Oct 9, 2018

View reviewed changes

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Oct 9, 2018

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. owner_pr This PR is made by the module's maintainer. labels Oct 9, 2018

maxamillion force-pushed the bugfix/46603-yum-config-empty branch from 21d1941 to ee72c62 Compare October 10, 2018 17:55

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Oct 10, 2018

add conf_file test cases to yum integration tests

1db261c

Signed-off-by: Adam Miller <admiller@redhat.com>

maxamillion force-pushed the bugfix/46603-yum-config-empty branch from ee72c62 to 1db261c Compare October 10, 2018 19:13

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 10, 2018

maxamillion merged commit fb6e91b into ansible:devel Oct 11, 2018

maxamillion mentioned this pull request Oct 11, 2018

yum module properly check for None config_file (#46641) #46820

Merged

tamsky reviewed Oct 12, 2018

View reviewed changes

dagwieers added the packaging Packaging category label Mar 3, 2019

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

yum module properly check for None config_file #46641

yum module properly check for None config_file #46641

maxamillion commented Oct 8, 2018

ansibot commented Oct 8, 2018

tamsky commented Oct 8, 2018

tamsky commented Oct 9, 2018

mkrizek Oct 9, 2018

maxamillion Oct 9, 2018

maxamillion commented Oct 9, 2018

tamsky Oct 9, 2018

maxamillion Oct 10, 2018

maxamillion Oct 10, 2018

tamsky Oct 12, 2018

mattclay commented Oct 9, 2018

jctanner commented Oct 11, 2018

maxamillion commented Oct 11, 2018

maxamillion commented Oct 11, 2018

tamsky left a comment

tamsky Oct 12, 2018

yum module properly check for None config_file #46641

yum module properly check for None config_file #46641

Conversation

maxamillion commented Oct 8, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ansibot commented Oct 8, 2018

tamsky commented Oct 8, 2018

tamsky commented Oct 9, 2018

mkrizek Oct 9, 2018

Choose a reason for hiding this comment

maxamillion Oct 9, 2018

Choose a reason for hiding this comment

maxamillion commented Oct 9, 2018

tamsky Oct 9, 2018

Choose a reason for hiding this comment

maxamillion Oct 10, 2018

Choose a reason for hiding this comment

maxamillion Oct 10, 2018

Choose a reason for hiding this comment

tamsky Oct 12, 2018

Choose a reason for hiding this comment

mattclay commented Oct 9, 2018

jctanner commented Oct 11, 2018

maxamillion commented Oct 11, 2018

maxamillion commented Oct 11, 2018

tamsky left a comment

Choose a reason for hiding this comment

tamsky Oct 12, 2018

Choose a reason for hiding this comment