-
Notifications
You must be signed in to change notification settings - Fork 266
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
Use json output for snapper #19281
Use json output for snapper #19281
Conversation
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment about use of global variable. Otherwise LGTM. This will need some VR's ofc.
f230215
to
8939f12
Compare
VRs updated |
8939f12
to
04c7049
Compare
# but other data which was written to serial device. We have to ensure | ||
# that we got what we expect. See poo#25716 | ||
sub _get_last_snap_number_old { | ||
# get snapshot id column, parse output in perl to avoid SIGPIPE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole function can be done in a single awk call:
snapper list --disable-used-space | awk -F '|' 'NR == 1 { for(i=1; i <= NF; ++i) if (match($i, /^ *# *$/)) colno = i; } END { print strtonum($colno); }'
Edit: There's a bug, fixing... Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not doubt that, but I am not sure whether we want to invest much time to code that is meant to be deleted. Maybe we can do that in another PR and merge this one for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not doubt that, but I am not sure whether we want to invest much time to code that is meant to be deleted. Maybe we can do that in another PR and merge this one for now
Makes the legacy code shorter, but I do get the point of not touching that code at all. There have to be VRs anyway, so might be worth it 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the thing, the VRs are already done for now. If we need to add another set or another round then yes, I can replace that code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the thing, the VRs are already done for now.
The one for TW is red, at least snapper_used_space
needs to be adjusted as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the used space column was missing in the basic list output. Lemme check it once more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
04c7049
to
c5c85d0
Compare
VR for the |
58460db
to
4ea6592
Compare
Newer version of snapper support json output. It is more convenient for parsing the data in snapper tests. - ticket: https://progress.opensuse.org/issues/160026
4ea6592
to
431e029
Compare
Hey, seems like this broke 12 SP3 tests: https://openqa.suse.de/tests/14316602#step/snapper_create/40 |
Linking fix PR in case someone ends up here looking at old tests :) #19318 |
Newer version of snapper support json output. It is more convenient for parsing the data in snapper tests.
Verification runs