-
Notifications
You must be signed in to change notification settings - Fork 449
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 vSphere host resource #836
Conversation
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.
Looks awesome so far! Have a few questions and comments. Many are just about consistency with the rest of the providers style.
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.
Just a few minor requests and questions.
@@ -51,7 +51,7 @@ func FromID(client *govmomi.Client, id string) (*object.HostSystem, error) { | |||
defer cancel() | |||
hs, err := finder.ObjectReference(ctx, ref) | |||
if err != nil { | |||
return nil, fmt.Errorf("could not find host system with id: %s: %s", id, err) | |||
return nil, 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.
Why the change 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.
Because the error messages coming from the API can be a bit vague sometimes. Adding a little bit of text gives better context and we can also track the source of the error just by grepping for the error message if we have to :) .
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.
You went the other way here though, right? It looks like it originally included "could not find host system with..." and that part was removed.
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.
Hmm you're right.... I guess I read the diff the other way. This must have happened by accident. The good news is that I have another branch I'm working on where I'm revisiting all this code and adding some text instead of just returning the vsphere error, so this will be adressed there.
vsphere/resource_vsphere_host.go
Outdated
return fmt.Errorf("Host addition failed. %s", err) | ||
} | ||
|
||
log.Printf("[DEBUG] Task Result: %#v", taskResult) |
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 think its generally better to avoid the fully verbose interpolation (especially with vSphere since it can get quite verbose), but there are certainly cases where it makes sense. Just want to make sure its purposeful 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.
Oops :) Yeah these are not left like that purpose. I'll do a quick sweep to be sure I haven't left any others like that.
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.
done
log.Printf("[DEBUG] Task Result: %#v", taskResult) | ||
var hostID string | ||
taskResultType := taskResult.(types.ManagedObjectReference).Type | ||
switch taskResultType { |
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.
ComputeResource
and HostSystem
should be sufficient, but I'd recommend a default
with an error about an unexpected type just in case.
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.
done
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. Nice feature addition!
This is a first draft of a resource that allows terraform users to add new ESXi hosts to a vsphere cluster and manage things like: [x] connection status [x] maintenance status [x] lockdown mode Once we settle on what it should look like we can work on adding more features.
cfb050a
to
8ae9ccf
Compare
(Squashed all commits into one) |
This is a first draft of a resource that allows terraform users to add new ESXi hosts to a vsphere cluster and manage things like:
[x] connection status
[x] maintenance status
[x] lockdown mode
Once we settle on what it should look like we can work on adding more features.