-
Notifications
You must be signed in to change notification settings - Fork 452
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
r/virtual_disk: update tests #635
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.
LGTM once the acceptance testing output is shown as passing
datastore = "${data.vsphere_datastore.ds.name}" | ||
} | ||
`, | ||
os.Getenv("VSPHERE_DATACENTER"), |
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: It might be more clear to reference these in the acceptance test functions themselves so its near the PreCheck
declaration and then require the variables be passed into the wrapping function for the fmt.Sprintf()
so its not possible to forget them, but that's generally how we do it in the AWS provider. Either way is probably fine!
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 actually like your way more as it would make the config functions more reusable. At this point, I'm going to just stick with this way just to keep it consistent with the other vsphere tests.
|
No description provided.