-
Notifications
You must be signed in to change notification settings - Fork 112
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 NSX-T support for Org Vdc #550
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.
Tests were successful in 10.1 and 10.2
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.
Few small comments, LGTM otherwise!
vcd/config_test.go
Outdated
func skipNoNsxtConfiguration(t *testing.T) { | ||
generalMessage := "Missing NSX-T config: " | ||
if testConfig.VCD.NsxtProviderVdc.Name == "" { | ||
t.Skip(generalMessage + "No provider vdc specified") |
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.
t.Skip(generalMessage + "No provider vdc specified") | |
t.Skip(generalMessage + "No provider VDC specified") |
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 was fixed. Didn't comment.
.changes/v2.10.0/550-features.md
Outdated
@@ -0,0 +1 @@ | |||
* Add support for NSX-T Org Vdc [GH-550] |
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 it's better to use capital "VDC" in human-facing messages.
* Add support for NSX-T Org Vdc [GH-550] | |
* Add support for NSX-T Org VDC [GH-550] |
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 was fixed as well. Comment was missing.
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
Also if possible in govcd, then here also would be good that not only configuration decides to run test or not.
vcd/org_vdc_shared_test.go
Outdated
@@ -0,0 +1,335 @@ | |||
// +build vdc nsxt ALL functional |
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.
maybe is better to rename file to something like vdc_util_test.go
vdc_common_test.go
to avoid opening/read this as normal/running test as others are with same naming convention.
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.
Renamed to vdc_common_test.go
This PR plays on top of recent default API version bump to V32.0 and tests out that NSX-T backed Org Vdc can be created.
It also adds a test for NSX-T backed Org Vdc when NSX-T related configuration is specified in config.
Note Additional NSX-T related configuration is mandatory to run the test
TestAccVcdOrgVdcNsxt