Use zypper instead of yum #1041

sbernhard · 2017-03-20T21:00:21Z

Use zypper library instead of yum on SUSE based distributions.
The zypper library transactions are running within a separate process as the zypper application would otherwise be blocked as long as the gofer process is running. Looks like that this is a issue in zypper / python swig implementation. Answer from zypper development mailing list:

Older libzypp versions are not able to drop the lock at runtime.
(and they also don't have 'zypp.ZYppFactory_instance().haveZYpp()'
to test whether the instance is present.)

On older distros you need to run zypp in a separate process,
so libzypp gets unloaded and releases the lock.

pulpbot · 2017-03-20T21:01:02Z

Can one of the admins verify this patch?

pulpbot · 2017-03-20T21:01:16Z

Can one of the admins verify this patch?

bmbouter · 2017-06-14T18:13:16Z

handlers/pulp_rpm/handlers/rpmtools_zypper.py

+        resolved = []
+        failed = []
+
+        # for t in ts_info:


Can this commented block be removed please.

bmbouter · 2017-06-14T18:15:17Z

handlers/pulp_rpm/handlers/rpmtools_zypper.py

+        log.info('Progress [%s]:\n%s', self.steps, self.details)
+
+
+# vim: set ts=4 sts=4 sw=4 et ai:


Can this be removed please?

bmbouter · 2017-06-14T18:16:08Z

handlers/pulp_rpm/handlers/rpmtools_zypper.py

+    def update(self, names=()):
+        """
+        Update installed packages.
+        When (names) is not specified, all packages are updated.


Would it be ok to separate the parameters from the docstring body with one line of whitespace. Here and similar elsewhere?

It would be OK but the rpmtools_yum.py doesn't do that. Therefore I think it's better be similar to rpmtools_yum.py and let it as it is.

bmbouter · 2017-06-14T18:17:17Z

handlers/pulp_rpm/handlers/rpmtools_zypper.py

+
+    def update(self, names=()):
+        """
+        Update installed packages.


Can a line of whitespace be added after the first line for any multi-line docstring? That is part PEP 257 and makes it more readable.

It would be OK but the rpmtools_yum.py doesn't do that. Therefore I think it's better be similar to rpmtools_yum.py and let it as it is.

bmbouter · 2017-06-14T18:18:15Z

handlers/pulp_rpm/handlers/rpmtools_zypper.py

+
+        return result
+
+    def update_p(self, names, q):


I suspect this doesn't have a docstring because it's an internal method. If so, can it become a private method such as _update_p. All public methods should have docstrings.

bmbouter · 2017-06-14T18:18:56Z

handlers/pulp_rpm/handlers/rpmtools_zypper.py

+    log.addHandler(ch)
+
+
+class Zypp:


It would be great if we could get some more docstrings on these methods.

bmbouter · 2017-06-14T18:25:22Z

This is a great feature, can a few things be done to track it's addition to Pulp?

Create a Redmine Story describing what it does for Pulp RPM. That can be filed here.
Redo the commit so that it references the issue you make ^. Here are docs on what that looks like.
Add documentation somehow documenting this capability being added to pulp_rpm. The docs are here and compile with make clean;make html.
Add a release note for the feature. If we can merge it before 2.14 becomes a beta then add those release notes to the 2.14. The 2.14 release schedule is here.

bmbouter · 2017-06-14T18:26:43Z

Can you ensure you are in the AUTHORS file? Also can the commits be squashed so there is only 1 commit?

bmbouter

@sbernhard Thank you so much for this pull request. I apologize it's gone unreviewed for so long. I want to help get it merged, so I left the things that need to be done. It's mostly documentation and docstrings, but check out the comments I left. If there are any questions or ideas please let me know so we can help. This is going to be a great addition for the community.

bmbouter · 2017-06-14T18:32:20Z

handlers/pulp_rpm/handlers/rpmtools_zypper.py

+        # Now all installed and available items are in the pool:
+        log.debug("Known items: %d" % (self.Z.pool().size()))
+
+    def poolInstall(self, capstr):


What do these pool*() methods do?

bmbouter · 2017-06-14T18:32:48Z

handlers/pulp_rpm/handlers/rpmtools_zypper.py

+
+        return result
+
+    def poolCommit(self, dryrun):


What does this do?

dralley · 2017-06-19T14:06:11Z

I noticed that these lines are repeated in several (7) places in this PR. Would you mind pulling this out into a documented helper method?

# TODO: looks like that this is not necessary for package groups. Only str() is required!
names = [str(x.replace('-*-*.*', '').replace('*:', '')) for x in names]

asmacdo · 2017-06-19T14:07:04Z

handlers/pulp_rpm/handlers/rpmtools_zypper.py

+
+        return result
+
+    def update_minimal(self, advisories=[]):


Default values should not be mutable. This will need to be changed in other places as well.

http://python-guide-pt-br.readthedocs.io/en/latest/writing/gotchas/#mutable-default-arguments

dralley · 2017-06-19T14:07:42Z

handlers/pulp_rpm/handlers/rpmtools_zypper.py

+        result = None
+
+        # TODO: looks like that this is not necessary for "update_minimal". Only str() is required!
+        advisories = [str(x.replace('-*-*.*', '').replace('*:', '')) for x in advisories]


I noticed that these lines are repeated multiple times throughout this PR. Could they be pulled out into a separate helper method?

pulpbot · 2017-06-21T17:51:17Z

Can one of the admins verify this patch?

sbernhard mentioned this pull request Jun 14, 2017

Test zypper integration in pulp_rpm pulp/pulp-smash#674

Closed

bmbouter reviewed Jun 14, 2017

View reviewed changes

bmbouter requested changes Jun 14, 2017

View reviewed changes

bmbouter reviewed Jun 14, 2017

View reviewed changes

handlers/pulp_rpm/handlers/rpmtools_zypper.py Outdated

return result

def poolCommit(self, dryrun):

Copy link

Member

bmbouter Jun 14, 2017

Choose a reason for hiding this comment

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

What does this do?

dralley self-requested a review June 19, 2017 14:06

asmacdo reviewed Jun 19, 2017

View reviewed changes

dralley requested changes Jun 19, 2017

View reviewed changes

sbernhard closed this Sep 1, 2017

sbernhard force-pushed the master branch from fb5f34b to 15bf585 Compare September 1, 2017 13:37

sbernhard mentioned this pull request Sep 1, 2017

- Use zypper instead of yum #1062

Merged

ThikaXer mentioned this pull request Nov 30, 2020

Add support for synchronizing oracle repos using ULN #1896

Merged

Use zypper instead of yum #1041

Use zypper instead of yum #1041

sbernhard commented Mar 20, 2017 •

edited

pulpbot commented Mar 20, 2017

pulpbot commented Mar 20, 2017

bmbouter Jun 14, 2017

bmbouter Jun 14, 2017

bmbouter Jun 14, 2017

sbernhard Sep 1, 2017

bmbouter Jun 14, 2017

sbernhard Sep 1, 2017

bmbouter Jun 14, 2017

bmbouter Jun 14, 2017

bmbouter commented Jun 14, 2017

bmbouter commented Jun 14, 2017

bmbouter left a comment

bmbouter Jun 14, 2017

bmbouter Jun 14, 2017

dralley commented Jun 19, 2017 •

edited

asmacdo Jun 19, 2017

dralley Jun 19, 2017

pulpbot commented Jun 21, 2017

		log.info('Progress [%s]:\n%s', self.steps, self.details)


		# vim: set ts=4 sts=4 sw=4 et ai:

Use zypper instead of yum #1041

Use zypper instead of yum #1041

Conversation

sbernhard commented Mar 20, 2017 • edited

pulpbot commented Mar 20, 2017

pulpbot commented Mar 20, 2017

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

bmbouter commented Jun 14, 2017

bmbouter commented Jun 14, 2017

bmbouter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley commented Jun 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pulpbot commented Jun 21, 2017

sbernhard commented Mar 20, 2017 •

edited

dralley commented Jun 19, 2017 •

edited