-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Adding 'dist-upgrade' support to the zypper module #37088
Adding 'dist-upgrade' support to the zypper module #37088
Conversation
# Creates a solver test case for debugging. | ||
log.info('Executing debugsolver and performing a dry-run dist-upgrade') | ||
cmd_debugsolver = cmd_update + ['--debug-solver'] | ||
__zypper__.noraise.call(*cmd_debugsolver) |
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 would not introduce another variable here just for sake to have it.
__zypper__.noraise.call(*cmd_update + ['--debug-solver'])
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.
As I understand this is an extra-call in case of dry-run, additionally to that one below, right?
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.
Yes. It's an extra call in case of dry-run.
if fromrepo: | ||
fromrepoopt = ['--from', fromrepo] | ||
log.info('Targeting repo {0!r}'.format(fromrepo)) | ||
cmd_update.extend(fromrepoopt) |
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 would prefer logger after everything. So after cmd_update...
.
cmd_update.extend(fromrepoopt) | ||
|
||
if novendorchange: | ||
log.info('Disabling changes of vendor') |
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 log message would be better after the cmd_update
.
''' | ||
ret = {'changes': {}, | ||
'result': True, | ||
'comment': '', | ||
} | ||
|
||
cmd_update = ['dist-upgrade'] if dist_upgrade else ['update'] | ||
cmd_update.extend(['--auto-agree-with-licenses']) |
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.
No need in extra brackets and it can be shorter. This is sufficient:
cmd_update = (['dist-upgrade'] if dist_upgrade else ['update']) + ['--auto-agree-with-licenses']
if refresh: | ||
refresh_db() | ||
|
||
if dryrun: | ||
cmd_update.extend(['--dry-run']) |
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.
No need extra brackets and .append()
is just sufficient here.
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.
Additionally to the above, please also take a look at this one.
self.assertFalse(ret['result']) | ||
self.assertEqual(ret['comment'], zypper_out.strip()) | ||
self.assertDictEqual(ret['changes'], {}) | ||
zypper_mock.noraise.call.assert_called_with('dist-upgrade', '--auto-agree-with-licenses', '--from', 'DUMMY') |
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.
Additionally to the previous things, I have no idea what had happened to your editor so the indentation went nuts. :-)
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.
Seems OK now. Thanks!
Thanks for your suggestions @isbm ! |
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.
Sorry. One more. :-)
|
||
if novendorchange: | ||
cmd_update.append('--no-allow-vendor-change') | ||
log.info('Disabling changes of vendor') |
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.
Oops, missed this one. "Disabling vendor changes" is probably more correct.
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.
Ok! Adding.. :-)
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.
Cool, thanks!
What does this PR do?
This PR adds
dist-upgrade
support for zypperpkg.upgrade
. It allows you to performdryrun
upgrades, and also setfromrepo
andnovendorchange
options when you're performing a systemdist-upgrade
.If possible, this changes should also go everywhere 😄
Thanks!
Tests written?
Yes
/cc @isbm