-
Notifications
You must be signed in to change notification settings - Fork 558
Conversation
can we add tests? |
switch vmProvisioningState { | ||
case "Creating": | ||
fallthrough | ||
case "Updating": |
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.
these can be written more compactly:
case "Creating", "Updating", "Succeeded":
Really a matter of choice though. @jackfrancis @amanohar any opinion on which style we want?
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.
+1 for the succinct style. See the "Multiple Cases" 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.
sure, I will change that.
@weinong We would need to overhaul current ARM simulation framework. I will create a task and we will do that in a separate PR. |
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.
First pass LGTM. I'm assuming you tested the change in the bug scenario
case <-timeoutTimer.C: | ||
retryTimer.Stop() | ||
kan.logger.Errorf("Node was not ready within %v", timeout) | ||
return fmt.Errorf("Node was not ready within %v", timeout) |
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.
[nit] err := fmt.Errorf("Node was not ready within %v", timeout")
kan.logger.Error(err.Error())
return err
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 reup
fixing agent pool upgrade logic