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

add zypper arguments to snapshot description #315

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

kmotavalli
Copy link

update scripts/zypp-plugin.py to extend the snapshot description with zypper cmdline arguments when zypp is run from zypper. could be made universal by parsing TransactionStepList, maybe

update scripts/zypp-plugin.py to extend the snapshot description with zypper cmdline arguments when zypp is run from zypper. could be made universal by parsing TransactionStepList, maybe
@kmotavalli
Copy link
Author

example output from snapper list:

http://pastebin.com/raw/SHja6NEn

@NicoHood
Copy link
Contributor

NicoHood commented Jan 6, 2017

That is a really good idea. snap-pac does the same for ArchLinux:

single | 2596 |       | Thu 29 Dec 2016 12:00:14 AM CET | root | timeline | timeline                                             |                                                                 
pre    | 2615 |       | Thu 29 Dec 2016 06:21:54 PM CET | root | number   | pacman -Syu                                          |                                                                 
post   | 2616 | 2615  | Thu 29 Dec 2016 06:22:22 PM CET | root | number   | pacman -Syu                                          |                                                                 
pre    | 2621 |       | Thu 29 Dec 2016 10:56:44 PM CET | root | number   | pacman -U snapper-0.4.1-1-x86_64.pkg.tar.xz          |                                                                 
post   | 2622 | 2621  | Thu 29 Dec 2016 10:56:45 PM CET | root | number   | pacman -U snapper-0.4.1-1-x86_64.pkg.tar.xz          |                                                                 
single | 2625 |       | Fri 30 Dec 2016 01:00:14 AM CET | root | timeline | timeline                                             |                                                                 
single | 2627 |       | Fri 30 Dec 2016 11:33:15 AM CET | root | number   | boot                                                 |   

@@ -153,7 +159,7 @@ def PLUGINBEGIN(self, headers, body):

logging.debug("headers: %s" % headers)

self.description = "zypp(%s)" % basename(readlink("/proc/%d/exe" % getppid()))
self.description = "zypp(%s) %s" % (basename(readlink("/proc/%d/exe" % getppid())), self.zypper_arguments())
Copy link
Member

Choose a reason for hiding this comment

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

AFIAS the description has a trailing whitespace if it's not from zypper. That's not nice.

@aschnell
Copy link
Member

In general the patch looks OK.

But from my experience the command line for zypper can be very long (spanning several lines) and that will make the output of snapper unreadable (see #268). Could you make adding the arguments optional (controlled via zypp-plugin.conf)?

@NicoHood
Copy link
Contributor

Instead of loosing information I'd rather fix #268 instead.

only append zypper command line arguments to the snapshot description if 
<zypper-extended-description enabled="true"> is set inside <description> in <snapper-zypp-plugin-conf>.

set the element inside zypper-extended-description to N in order to cut the zypper commandline description after N chars. defaults to 0 (unlimited output).
<description>
  <zypper-extended-description enabled="true">32</zypper-extended-description>
</description>
@kmotavalli
Copy link
Author

ok, I tried to make the required changes

@@ -17,7 +17,7 @@
</solvables>

<!-- Set enabled to "true" in order to save zypper commandline arguments in snapshot descriptions.
By default output the arguments (not including zypp(zypper) ) is truncated to 32 chars. Set to 0 for unlimited output. -->
by default argoments following zypp(zypper) are truncated at 32 chars. change to 0 for unlimited output -->
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in arguments

Copy link
Author

Choose a reason for hiding this comment

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

thanks

@@ -56,6 +56,9 @@ class Config:

def __init__(self):
self.solvables = []
self.zypper_extended_description = []
self.zypper_extended_description.append("false")
self.zypper_extended_description.append("0")
Copy link
Member

Choose a reason for hiding this comment

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

Don't use an array to store two values of different type and meaning. Just use e.g. self.zypper_extended_description_enable = false and self.zypper_extended_description_length = 0.

self.description = "zypp(%s)" % basename(readlink("/proc/%d/exe" % getppid()))
if config.zypper_extended_description[0] != "true":
self.description = "zypp(%s)" % basename(readlink("/proc/%d/exe" % getppid()))
elif config.zypper_extended_description[0] == "true":
Copy link
Member

Choose a reason for hiding this comment

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

A simple else is enough here.

if config.zypper_extended_description[0] != "true":
self.description = "zypp(%s)" % basename(readlink("/proc/%d/exe" % getppid()))
elif config.zypper_extended_description[0] == "true":
self.description = "zypp(%s)%s" % (basename(readlink("/proc/%d/exe" % getppid())), self.zypper_arguments())
Copy link
Member

Choose a reason for hiding this comment

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

The basename(readlink(... code is twice here. You could do something like:

self.description = basename(readlink("/proc/%d/exe" % getppid()))
if zypper_extended_description_enabled
    self.description.append(self.zypper_arguments())

@kmotavalli
Copy link
Author

ok, thanks for the suggestions

@@ -56,6 +56,8 @@ class Config:

def __init__(self):
self.solvables = []
self.self.zypper_extended_description_enabled = "false"
Copy link
Member

Choose a reason for hiding this comment

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

Please use False. Python has booleans.

loggin.error("unknown extended-config enabled attribute %s" % description_enabled)
continue
if description_enabled == "true":
self.zypper_extended_description_enabled = "true"
Copy link
Member

Choose a reason for hiding this comment

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

Use True.

argument = " " + " ".join(open("/proc/%s/cmdline" % getppid()).read().split('\x00')[1:])
else:
return ""
if config.zypper_extended_description_length == "0":
Copy link
Member

Choose a reason for hiding this comment

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

Just plain 0.

try:
for tmp3 in dom.getElementsByTagName("description"):
for tmp4 in tmp3.getElementsByTagName("zypper-extended-description"):
string_size = tmp4.childNodes[0].data
Copy link
Member

Choose a reason for hiding this comment

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

Convert to int here instead of below.

@kmotavalli
Copy link
Author

were the required fixes ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants