#19587 add yum skip-broken #21475

vmindru · 2017-02-15T16:42:49Z

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

modules/packaging/os/yum.py

ANSIBLE VERSION

targeting 2.3 i guess

SUMMARY

adding skip_broken option, --skip-broken as per yum man is a general option meaning it can be added all other the place, therefore, adding it into the yum_basecmd

bonus: did some pep8 refactor

test-module pastebin http://pastebin.com/mgCmcVdq

PNTSYSOPS-1901 - internal reference Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

vmindru · 2017-02-15T16:43:47Z

this WIP need to test this , will bump the PR once this is ready for review

things at line 646 look ugly .. trying to make them look a bit more human readble though the entire approach should be rewriten PNTSYSOPS-1901 - internal reference Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

vmindru · 2017-02-15T16:58:33Z

i hope the thingy will not complain about this 3 vars , i was thining to cover those in a separte MR

[vmindru@vmutil ansible-devel]$ flake8 lib/ansible/modules/packaging/os/yum.py 
lib/ansible/modules/packaging/os/yum.py:829:13: F841 local variable 'is_group' is assigned to but never used
lib/ansible/modules/packaging/os/yum.py:1014:17: F841 local variable 'msg' is assigned to but never used
lib/ansible/modules/packaging/os/yum.py:1128:29: F841 local variable 'a' is assigned to but never used
[vmindru@vmutil ansible-devel]$

vmindru · 2017-02-16T07:38:14Z

imho this is ready for review, there are this 2 issues not sure if i should remove them within this MR or as a separate one.

lib/ansible/modules/packaging/os/yum.py:829:13: F841 local variable 'is_group' is assigned to but never used
lib/ansible/modules/packaging/os/yum.py:1128:29: F841 local variable 'a' is assigned to but never used
[vmindru@vmutil ansible-devel]$

e.g. IMHO this entire conditional makes no use, it's either redundant or became redundant during some change.

1126                         if not i in current_repos:                                                                                                              
1127                             rid = my.repos.getRepo(i)                                                                                                           
1128                             a = rid.repoXML.repoid  ##  if no one complains remove this on next MR

same goes here this peace can be rewrote

 825     for pkg in items:                                                                                                                                           
 826         is_group = False                                                                                                                                        
 827         # group remove - this is doom on a stick                                                                                                                
 828         if pkg.startswith('@'):                                                                                                                                 
 829             is_group = True                                                                                                                                                                                    
 830         else:                                                                                                                                                   
 831             if not is_installed(module, repoq, pkg, conf_file, en_repos=en_repos, dis_repos=dis_repos, installroot=installroot):                                
 832                 res['results'].append('%s is not installed' % pkg)                                                                                              
 833                 continue

might be end-up like this

 825     for pkg in items and is_installed(module, repoq, pkg, conf_file, en_repos=en_repos, dis_repos=dis_repos, installroot=installroot):                          
 826         res['results'].append('%s is not installed' % pkg)                                                                                                                                                     
 827         continue

vmindru

just couple of comments

vmindru · 2017-02-16T11:47:04Z

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

@@ -988,10 +1011,13 @@ def latest(module, items, repoq, yum_basecmd, conf_file, en_repos, dis_repos, in
        for w in will_update:
            if w.startswith('@'):
                to_update.append((w, None))
-                msg = '%s will be updated' % w
+                res['msg'] += '%s will be updated' % w


not even sure why that previous msg = '%s will be updated' % w was there, just tried to 'guess'

Judging from the other things in this conditional, it probably should be appended to to_update?

in this case I think it should be removed at all cause to_updated get's updated on previous line

vmindru · 2017-02-16T11:47:53Z

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

@@ -1095,7 +1125,7 @@ def ensure(module, state, pkgs, conf_file, enablerepo, disablerepo,
                    for i in new_repos:
                        if not i in current_repos:
                            rid = my.repos.getRepo(i)
-                            a = rid.repoXML.repoid
+                            a = rid.repoXML.repoid  ##  if no one complains remove this on next MR 


this is one of the var complaints, the var is assigned but not really used.

Check the yum code for sideeffects (repoid might be a property and referencing it might cause some action to happen). If this is a simply attribute rather than something producing side-effects, feel free to get rid of it.

3 import yum 4 from json import dumps 5 6 instance = yum.YumBase() 7 print "###################################" 8 for repo in instance.repos.listEnabled(): 9 print type(repo.repoXML.repoid)

says

################################### Loaded plugins: product-id Repo rhel-7-server-satellite-tools-6.2-rpms forced skip_if_unavailable=True due to: /etc/pki/entitlement/7092233073807600698-key.pem Repo rhel-server-rhscl-7-eus-source-rpms forced skip_if_unavailable=True due to: /etc/pki/entitlement/7092233073807600698-key.pem Repo rhel-7-server-extras-rpms forced skip_if_unavailable=True due to: /etc/pki/entitlement/7092233073807600698-key.pem Repo rhel-7-server-optional-rpms forced skip_if_unavailable=True due to: /etc/pki/entitlement/7092233073807600698-key.pem Repo rhel-7-server-rpms forced skip_if_unavailable=True due to: /etc/pki/entitlement/7092233073807600698-key.pem Repo rhel-7-fast-datapath-rpms forced skip_if_unavailable=True due to: /etc/pki/entitlement/7092233073807600698-key.pem Repo rhel-7-server-tus-rpms forced skip_if_unavailable=True due to: /etc/pki/entitlement/7092233073807600698-key.pem <type 'str'> <type 'str'> <type 'str'> <type 'str'> <type 'str'> <type 'str'> <type 'str'> <type 'str'> <type 'str'> <type 'str'> <type 'str'> <type 'str'> <type 'str'> [vmindru@vmutil python]$

so i guess it's a string.

~

will remove that, it's basically repo name string.

abadger · 2017-02-16T17:04:57Z

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

+                                                             stuff,
+                                                             conf_file,
+                                                             qf=qf,
+                                                             installroot=installroot)) if p.strip() ]


This is horrible formatting ;-) If you want to break this up, make logical divisions more like this:

return [ pkg_to_dict(p) for p in sorted( is_installed(module, repoq, stuff, conf_file, qf=is_installed_qf, installroot=installroot) + is_available(module, repoq, stuff, conf_file, qf=qf, installroot=installroot)) if p.strip() ]

ack, will do was not sure how do format this 100 organizations 100 formatting styles :-D

abadger · 2017-02-16T17:10:03Z

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

-    #   list=available
-    #   list=repos
-    #   list=pkgspec
-


Leave this, at least for now. there's no other listing of the two types of commands (and the arguments that list takes aren't even i nthe documentatoin :-(

abadger · 2017-02-16T17:12:58Z

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

@@ -1058,6 +1084,10 @@ def ensure(module, state, pkgs, conf_file, enablerepo, disablerepo,

    dis_repos =[]
    en_repos = []
+
+    if skip_broken:
+        yum_basecmd.extend(['--skip-broken'])


Is skip-broken allowed for all of the commands that we run? (I've used it with yum update but never with yum remove or yum install).

Yep https://linux.die.net/man/8/yum --skip-broken is in the general section meaning it can be added to any other action.

we use it quite often during Maintenance windows and mass server updates, we don't want to waste time to solve problems on failed machines, we just --skip-update and later follow-up what are the deps issues.

relates to: revert comment deletion ansible#21475 (comment) relates to: remove irelevant var ansible#21475 (comment) relates to: reformat ansible#21475 (comment) Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

relates to: ansible#21475 (comment) Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

vmindru · 2017-02-16T19:21:22Z

can we ignore the CI test failure

2017-02-16 17:58:13 ERROR: PEP 8: lib/ansible/modules/packaging/os/yum.py: Passes current rule set. Remove from legacy list (test/sanity/pep8/legacy-files.txt).
2017-02-16 17:58:13 ERROR: PEP 8: There are 1 issues which need to be resolved.

I presume it's caused by, and i really don't want to change that part of code

[vmindru@vmutil ansible-devel]$ flake8 lib/ansible/modules/packaging/os/yum.py 
lib/ansible/modules/packaging/os/yum.py:819:13: F841 local variable 'is_group' is assigned to but never used
[vmindru@vmutil ansible-devel]$

EDIT:

seems this failure actually has to do with the fact that this is listed in test/sanity/pep8/legacy-files.txt if removed tests pass ok

http://pastebin.com/YhrNmhSJ

Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

mattclay · 2017-02-16T23:28:37Z

@vmindru Since you've fixed the PEP 8 issues that currently exist in the file, you need to remove the file from test/sanity/pep8/legacy-files.txt as part of your PR.

abadger · 2017-02-17T01:23:22Z

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

-                    for i in new_repos:
-                        if not i in current_repos:
-                            rid = my.repos.getRepo(i)
-                            a = rid.repoXML.repoid


I think accessing repoXML has a side effect. This is in the yum source (yumRepo.py):

def _loadRepoXML(self, text=None): """retrieve/check/read in repomd.xml from the repository""" try: return self._groupLoadRepoXML(text, self._mdpolicy2mdtypes()) except KeyboardInterrupt: self._revertOldRepoXML() # Undo metadata cookie? raise raise Errors.RepoError, 'Bad loadRepoXML policy (for %s): %s' % (self.ui_id, self.mdpolicy) def _getRepoXML(self): if self._repoXML: return self._repoXML self._loadRepoXML(text=self.ui_id) return self._repoXML repoXML = property(fget=lambda self: self._getRepoXML(), fset=lambda self, val: setattr(self, "_repoXML", val), fdel=lambda self: setattr(self, "_repoXML", None))

So we probably need to keep this. Should add a comment to say why it's there as well.

Note: If this is causing the pep8 test to complain, it would be a valid place to use the # nopep8 comment (along with the comment explaining why we have to do this).

this has been reverted, #21619 has been created not to forget to tackle this issue. I decided to take care of this issue in a separate PR to keep things less polluted.

abadger · 2017-02-17T01:26:39Z

This is close. In addition to removing from the pep8 skip file I only see one problem that needs to be addressed.

Relates to: ansible#21475 (review) Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

vmindru · 2017-02-18T17:30:47Z

w00t! :) all tests have passed LOL so nice to see green color all other the place. Now need final review and then I can tackle this module further under #21619

Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

abadger · 2017-02-21T15:59:34Z

Squashed and merged to devel (for 2.3.0 release). Thanks vmindru!

Veaceslav Mindru added 2 commits February 15, 2017 11:18

ansible#19587: refactor to PEP8

9e15d31

Issue: ansible#19587: add skip_broken option to yum module

7ac7ddb

PNTSYSOPS-1901 - internal reference Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

vmindru mentioned this pull request Feb 15, 2017

yum skip broken issue #19587

Closed

ansibot added affects_2.3 This issue/PR affects Ansible v2.3 core_review In order to be merged, this PR must follow the core review workflow. feature_pull_request module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. labels Feb 15, 2017

ansibot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Feb 15, 2017

gundalow changed the title ~~I19587 yum skip broken~~ [WIP] I19587 yum skip broken Feb 15, 2017

gundalow added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. and removed needs_triage Needs a first human triage before being processed. labels Feb 15, 2017

vmindru changed the title ~~[WIP] I19587 yum skip broken~~ [WIP] #19587 yum skip broken Feb 15, 2017

ansibot removed core_review In order to be merged, this PR must follow the core review workflow. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Feb 15, 2017

ansible#19587: fixing typo, skip-broken has to be passed as list

d2da1d1

ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Feb 16, 2017

vmindru commented Feb 16, 2017

View reviewed changes

vmindru changed the title ~~[WIP] #19587 yum skip broken~~ #19587 yum skip broken Feb 16, 2017

vmindru changed the title ~~#19587 yum skip broken~~ #19587 add yum skip-broken Feb 16, 2017

ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Feb 16, 2017

abadger reviewed Feb 16, 2017

View reviewed changes

PR: ansible#21475: peer review changes

52455b6

relates to: revert comment deletion ansible#21475 (comment) relates to: remove irelevant var ansible#21475 (comment) relates to: reformat ansible#21475 (comment) Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

PR: ansible#21475: peer review changes

9b3968f

relates to: ansible#21475 (comment) Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

Issue: ansible#21475: fixing typo in the Man section

2d0c1da

Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Feb 16, 2017

abadger reviewed Feb 17, 2017

View reviewed changes

PR: ansible#21475 - Peer review: revert variable a

6c8d199

Relates to: ansible#21475 (review) Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

Issue: 19587 - small correction

8249dd3

Signed-off-by: Veaceslav Mindru <vmindru@redhat.com> <mindruv@gmail.com>

abadger merged commit cdcdc1d into ansible:devel Feb 21, 2017

ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 4, 2018

dagwieers added the packaging Packaging category label Mar 3, 2019

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#19587 add yum skip-broken #21475

#19587 add yum skip-broken #21475

vmindru commented Feb 15, 2017 •

edited

vmindru commented Feb 15, 2017

vmindru commented Feb 15, 2017

vmindru commented Feb 16, 2017

vmindru left a comment

vmindru Feb 16, 2017

abadger Feb 16, 2017

vmindru Feb 16, 2017

vmindru Feb 16, 2017

abadger Feb 16, 2017

vmindru Feb 16, 2017

vmindru Feb 16, 2017

abadger Feb 16, 2017

vmindru Feb 16, 2017

abadger Feb 16, 2017

abadger Feb 16, 2017

vmindru Feb 16, 2017

vmindru Feb 16, 2017

vmindru commented Feb 16, 2017 •

edited

mattclay commented Feb 16, 2017

abadger Feb 17, 2017

abadger Feb 17, 2017

vmindru Feb 18, 2017

abadger commented Feb 17, 2017

vmindru commented Feb 18, 2017

abadger commented Feb 21, 2017

#19587 add yum skip-broken #21475

#19587 add yum skip-broken #21475

Conversation

vmindru commented Feb 15, 2017 • edited

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

SUMMARY

vmindru commented Feb 15, 2017

vmindru commented Feb 15, 2017

vmindru commented Feb 16, 2017

vmindru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmindru commented Feb 16, 2017 • edited

mattclay commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abadger commented Feb 17, 2017

vmindru commented Feb 18, 2017

abadger commented Feb 21, 2017

vmindru commented Feb 15, 2017 •

edited

vmindru commented Feb 16, 2017 •

edited