-
Notifications
You must be signed in to change notification settings - Fork 913
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
Add a command line option to change an existing disk attached to a VM #658
Conversation
…. Size and mode only.
@bastienbc, VMware has approved your signed contributor license agreement. |
@@ -0,0 +1,179 @@ | |||
/* | |||
Copyright (c) 2014-2015 VMware, Inc. All Rights Reserved. |
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 needs to be 2017
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.
Ah yes, also in the RDM 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.
Yes, just '2017' is fine here. Everything else looks good @bastienbc
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.
@bastienbc thanks, looks great, just a few review comments.
} | ||
return nil | ||
} | ||
|
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.
Please add a 'func (cmd *change) Description() string', the 'Examples' section in particular is most helpful.
} | ||
} | ||
|
||
switch { |
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.
switch len(disks) {
case 0:
...
case 1:
...
default:
}
for _, device := range list { | ||
switch md := device.(type) { | ||
case *types.VirtualDisk: | ||
switch { |
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 looks the same as CheckDiskProperties? If so, personally would prefer to see CheckDiskProperties return a bool and call it here to remove the duplicate code.
|
||
err = task.Wait(ctx) | ||
if err != nil { | ||
return errors.New(fmt.Sprintf("Error resizing main disk\nLogged Item: %s", 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.
should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...) (go-golint)
@@ -0,0 +1,179 @@ | |||
/* | |||
Copyright (c) 2014-2015 VMware, Inc. All Rights Reserved. |
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.
Ah yes, also in the RDM PR.
…e pull request made by dougm.
I needed the ability to resize a VM disk once it has been created, so I made a new command: "vm.disk.change".
It allows search of a disk using its name, label, key or even the file path to the vmdk.
It only changes the size, and the mode.