Skip to content

Clarify rollback log output#7830

Merged
zjs merged 2 commits intovmware:masterfrom
zjs:issue/6686
Apr 25, 2018
Merged

Clarify rollback log output#7830
zjs merged 2 commits intovmware:masterfrom
zjs:issue/6686

Conversation

@zjs
Copy link
Member

@zjs zjs commented Apr 24, 2018

When rolling back an upgrade, clearly log that we are doing so.

Fixes #6686

When rolling back an upgrade, clearly log that we are doing so.
@zjs zjs self-assigned this Apr 24, 2018
@zjs zjs requested review from a user, andrewtchin, cgtexmex, mhagen-vmware and stuclem April 24, 2018 23:04
@codecov-io
Copy link

codecov-io commented Apr 24, 2018

Codecov Report

Merging #7830 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7830   +/-   ##
=======================================
  Coverage   25.85%   25.85%           
=======================================
  Files          35       35           
  Lines        5124     5124           
=======================================
  Hits         1325     1325           
  Misses       3692     3692           
  Partials      107      107

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6341801...5445c26. Read the comment docs.

Copy link

@stuclem stuclem left a comment

Choose a reason for hiding this comment

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

LGTM


op.Infof("### Upgrading VCH ####")
var action management.Action
if !u.Data.Rollback {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you remove negation and swap if/else blocks?

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that. Usually I prefer to avoid negation, but in this case it seemed more intuitive to have the upgrade case first, in part for consistency with the rest of the file:

if !u.Data.Rollback {

if !u.Data.Rollback {

@zjs zjs merged commit 2c4dde9 into vmware:master Apr 25, 2018
zjs added a commit to zjs/vic that referenced this pull request Jun 21, 2018
When rolling back an upgrade, clearly log that we are doing so.

(cherry picked from commit 2c4dde9)
zjs added a commit to zjs/vic that referenced this pull request Jun 22, 2018
When rolling back an upgrade, clearly log that we are doing so.

(cherry picked from commit 2c4dde9)
zjs added a commit to zjs/vic that referenced this pull request Jun 28, 2018
When rolling back an upgrade, clearly log that we are doing so.

(cherry picked from commit 2c4dde9)
zjs added a commit to zjs/vic that referenced this pull request Jul 2, 2018
When rolling back an upgrade, clearly log that we are doing so.

(cherry picked from commit 2c4dde9)
zjs added a commit to zjs/vic that referenced this pull request Jul 2, 2018
When rolling back an upgrade, clearly log that we are doing so.

(cherry picked from commit 2c4dde9)
zjs added a commit to zjs/vic that referenced this pull request Jul 27, 2018
When rolling back an upgrade, clearly log that we are doing so.

(cherry picked from commit 2c4dde9)
zjs added a commit that referenced this pull request Jul 27, 2018
When rolling back an upgrade, clearly log that we are doing so.

(cherry picked from commit 2c4dde9)
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.

7 participants