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

Add support for NSX-V Edge Gateway DHCP relay settings #271

Merged
merged 5 commits into from
Dec 9, 2019

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Dec 4, 2019

This PR adds support for Edge Gateway DHCP relay setting management using 3 methods: UpdateDhcpRelay, GetDhcpRelay and ResetDhcpRelay

Note. Test suite passed on 9.5 and 10.

@Didainius Didainius added the enhancement New feature or request label Dec 4, 2019
@Didainius Didainius self-assigned this Dec 4, 2019
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.

Our existing files, which are working on NSX-V EG, are prefixed with nsxv_

Screen Shot 2019-12-05 at 10 58 23

Wouldn't renaming to nsxv_dhcprelay.go fit the above group well?

CHANGELOG.md Outdated
@@ -28,6 +28,8 @@
* Added IP set handling functions `CreateNsxvIpSet`, `UpdateNsxvIpSet`, `GetNsxvIpSetByName`,
`GetNsxvIpSetById`, `GetNsxvIpSetByNameOrId`, `GetAllNsxvIpSets`, `DeleteNsxvIpSetById`,
`DeleteNsxvIpSetByName` [#269](https://github.com/vmware/go-vcloud-director/pull/269)
* Added `UpdateDhcpRelay`, `GetDhcpRelay` and `ResetDhcpRelay()` methods for Edge Gatway DHCP relay
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
* Added `UpdateDhcpRelay`, `GetDhcpRelay` and `ResetDhcpRelay()` methods for Edge Gatway DHCP relay
* Added `UpdateDhcpRelay`, `GetDhcpRelay` and `ResetDhcpRelay` methods for Edge Gatway DHCP relay

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@Didainius
Copy link
Collaborator Author

Wouldn't renaming to nsxv_dhcprelay.go fit the above group well?

It does probably. I got split brain while looking at load balancers. But in the end they don't have NSXV anywhere. Changed.

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 now!

@Didainius Didainius requested a review from dataclouder December 9, 2019 08:31
check.Assert(err, IsNil)

parentEntity = vcd.org.Org.Name + "|" + vcd.vdc.Vdc.Name + "|" + vcd.config.VCD.EdgeGateway
PrependToCleanupList(check.TestName(), "dhcpRelayConfig", parentEntity, check.TestName())
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment about using PrependToCleanupList instead of Add, for future reference

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

LGTM, just one changed needed

Comment on lines 76 to 80
if err != nil {
return err
}

return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

just return err is sufficient

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed. Although this had its reason. It is about clearly distinguishing the "happy" and "bad" path on function.

Copy link
Contributor

@dataclouder dataclouder 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 67663ca into vmware:master Dec 9, 2019
@Didainius Didainius deleted the vcd_dhcp_relay branch December 9, 2019 12:50
@Didainius Didainius restored the vcd_dhcp_relay branch January 7, 2020 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants