Skip to content
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

Introduce OpenAPI based Org VDC network management functions for NSX-V and NSX-T #354

Merged
merged 17 commits into from
Feb 15, 2021

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Jan 22, 2021

This PR introduces:

  • OpenApiOrgVdcNetwork structure and type meant to handle Org VDC networks for both NSX-T and NSX-V backed.
  • Structure NsxtImportableSwitch meant for lookup "importable switches" (NSX-T segments) to be used for NSX-T Imported networks. Methods GetNsxtImportableSwitchByName and GetAllNsxtImportableSwitches aid that
  • Methods vdc.IsNsxt and vdc.IsNsxv help to verify which type of network provider this VDC is backed.

Additionally it ads:

  • nsxtVdc shorthand in TestVCD struct for testing
  • Two new config fields edgeGateway (to use existing edge gateway) and unusedSegment which is needed to test our Imported NSX-T Org VDC network
  • Converts VirtualQuantity int->int64 to avoid variable overflow with big sizes (Error Refreshing Vapp after VM Creation terraform-provider-vcd#626)

@Didainius Didainius marked this pull request as ready for review January 26, 2021 10:18
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a few nits :)

}

if len(allEdges) < 1 {
return nil, fmt.Errorf("%s: got 0 Org Vdc network by name '%s'", ErrorEntityNotFound, name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, fmt.Errorf("%s: got 0 Org Vdc network by name '%s'", ErrorEntityNotFound, name)
return nil, fmt.Errorf("%s: got zero Org Vdc networks by name '%s'", ErrorEntityNotFound, name)

case "":
queryParameters.Set("pageSize", "32")

// If pageSize is specified ensure it is not >32
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems misplaced.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in correct place. I might need to improve the comment then. The point is that above case "" sets pageSize to 32 if none was specified.
The default case will check what value was provided and if it is >32 it will set it to 32. This is needed because this particular endpoint throws an ugly error if pageSize>32

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so then the indentation is too big, as it should be inline with default: as right now it looks as if the comment belongs to the case "":

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Didainius Didainius merged commit d64d2b4 into vmware:master Feb 15, 2021
@Didainius Didainius deleted the pr-nsxt-routed-network branch February 15, 2021 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants