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

Neutron Ports: Add support for extra_dhcp_opts #533

Closed
wants to merge 8 commits into from

Conversation

bugosiphionah
Copy link
Contributor

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+0.1%) to 71.796% when pulling 6435e8f on bugosiphionah:issue_511 into 273b98b on gophercloud:master.

@bugosiphionah
Copy link
Contributor Author

@jtopjian This is ready for review.

@jtopjian
Copy link
Contributor

@bugosiphionah Thanks for working on this - this is good start.

The two source code files you found are correct. In the first one, it shows further information about extra_dhcp_opts being imported:

https://github.com/openstack/neutron/blob/e6dc39518ccfef79ecf9beae0b3a2c094b4c220b/neutron/agent/linux/dhcp.py#L24

which can be found here:

https://github.com/openstack/neutron-lib/blob/master/neutron_lib/api/definitions/extra_dhcp_opt.py#L25-L44

According to that file, extra_dhcp_opts looks to have a more structured format. So I think we will need to create a special type for this. Something like:

type ExtraDHCPOpts struct {
  OptName   string                `json:"opt_name"`
  OptValue  string                `json:"opt_value"`
  IPVersion gophercloud.IPVersion `json:"ip_version"`
}

I think running some commands in a devstack environment will help a lot with implementing this PR. For example:

$ neutron help port-create
$ neutron --debug port-create --extra-dhcp-opt opt_name=tftp-server,opt_value=192.168.0.3,ip_version=4 ...

Using --debug will display the HTTP request and response bodies which will help with adding this to Gophercloud.

You can find some examples of valid DHCP options here:

https://github.com/openstack/neutron/blob/779a8d31e7afcd20671e7a09309f68339471f1d9/neutron/tests/unit/agent/linux/test_dhcp.py#L773-L804

Let me know if you have any questions :)

@coveralls
Copy link

coveralls commented Oct 21, 2017

Coverage Status

Coverage increased (+0.3%) to 71.947% when pulling 198c6a4 on bugosiphionah:issue_511 into 273b98b on gophercloud:master.

@bugosiphionah
Copy link
Contributor Author

@jtopjian This is ready for review.

Copy link
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@bugosiphionah Nice work! The new ExtraDHCPOpts struct is exactly what I had in mind :)

Please see the comments I left.

In addition, I recommend either adding a new unit test for "Create" or modifying the existing Create unit test.

For the Update unit test, I recommend creating a whole new unit test so there are two unit tests for update: one with ExtraDHCPOpts specified and one without. This way, we can test if omitempty is working correctly.

For both Create and Update unit tests, I recommend adding some values to the ExtraDHCPOpts struct instead of testing an empty struct.

You'll have to modify existing create/update fixtures and make new fixtures.

Let me know if you have any questions or need any help.

Marker string `q:"marker"`
SortKey string `q:"sort_key"`
SortDir string `q:"sort_dir"`
ExtraDHCPOPTS []ExtraDHCPOpts `q:"extra_dhcp_opts"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure ExtraDHCPOpts is a valid option for list operations? Can you verify this by providing the Neutron code that shows it or the equivalent neutron command?

TenantID string `json:"tenant_id,omitempty"`
SecurityGroups *[]string `json:"security_groups,omitempty"`
AllowedAddressPairs []AddressPair `json:"allowed_address_pairs,omitempty"`
ExtraDHCPOPTS []ExtraDHCPOpts `json:"extra_dhcp_opts,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

ExtraDHCPOPTS should be ExtraDHCPOpts.

DeviceOwner string `json:"device_owner,omitempty"`
SecurityGroups *[]string `json:"security_groups,omitempty"`
AllowedAddressPairs *[]AddressPair `json:"allowed_address_pairs,omitempty"`
ExtraDHCPOPTS *[]ExtraDHCPOpts `json:"extra_dhcp_opts,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

ExtraDHCPOPTS should be ExtraDHCPOpts.

@@ -96,6 +103,9 @@ type Port struct {

// Identifies the list of IP addresses the port will recognize/accept
AllowedAddressPairs []AddressPair `json:"allowed_address_pairs"`

//A set of zero or more extra DHCP option pairs
ExtraDHCPOPTS []ExtraDHCPOpts `json:"extra_dhcp_opts"`
Copy link
Contributor

Choose a reason for hiding this comment

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

ExtraDHCPOPTS should be ExtraDHCPOpts.

@@ -54,6 +54,13 @@ type AddressPair struct {
MACAddress string `json:"mac_address,omitempty"`
}

//This is the sub-struct that represents extra DHCP option pairs
Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to make sure there is a space after // in comments or the comment will not be rendered as documentation in godoc.

Also instead of using "This is..." I recommend using the field name: "ExtraDHCPOpts represents extra..."

@jtopjian
Copy link
Contributor

This was implemented in #804

@jtopjian jtopjian closed this Jul 15, 2018
cardoe pushed a commit to cardoe/gophercloud that referenced this pull request Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants