Conversation
mislav
left a comment
There was a problem hiding this comment.
This looks like it's almost there, but I would challenge us to exercise as much of the upgrade logic as possible (i.e. most filesystem/API reads and comparisons) in dry-run mode.
pkg/cmd/extension/manager.go
Outdated
| return upToDateError | ||
| } | ||
| if dryRun { | ||
| return nil |
There was a problem hiding this comment.
I feel like this exits too soon in the dry-run mode. There is heavy logic ahead that I can imagine it would be good to exercise even in dry-run mode. A dry-run should ideally do as much as a normal run would, sans any mutations (which get printed instead of executed).
There was a problem hiding this comment.
@mislav I updated this PR to now exercise more of the logic during dry-run mode. I opted against printing mutations for two reasons. First, in extensions commands we already don't display what is happening behind the scenes and this decision felt like keeping consistency. Second, when using the --all flag the output did not look clean with multiple output lines per extension that would be upgraded.
c9db1aa to
bf34147
Compare
|
@mislav I updated this PR if you want to take another look 🙇. The increased logic exercising meant passing around the |
|
@mislav Updated the PR to reflect the addition of |
mislav
left a comment
There was a problem hiding this comment.
Awesome! Thanks for the quick update
pkg/cmd/extension/manager.go
Outdated
| m.dryRunMode = true | ||
| } | ||
|
|
||
| func (m *Manager) DisableDryRunMode() { |
There was a problem hiding this comment.
Is Disable called from anywhere?
There was a problem hiding this comment.
It is not right now, I added it mostly because it felt weird having a Enable function without the corresponding Disable function. Having said that, I don't know if we will ever need the ability to disable it so I will remove it for now. It is easy enough to add later.
This PR does two related things.
extension listcommand. It was slow and made what should be a simple command less usable.--dry-runflag toextension upgradecommand. This option combined with the--allflag gives the user the same functionality as the oldextension listcommand functionality, except now the update available checking has to be explicitly asked for and is better aligned in theextension upgradecommand.Closes #4220