Baremetal V1 API Ports by iurygregory · Pull Request #1453 · gophercloud/gophercloud

iurygregory · 2019-02-16T11:38:38Z

This PR covers the Ports API calls according to the Baremetal API reference

List Ports (GET)
Create Port (POST)
List Detailed Ports (GET)
Show Port Details (GET)
Update a Port (PATCH)
Delete Port (DELETE)

You can view the source code here

This commit add the Ports API calls according to [1] - List Ports (GET) - Create Port (POST) - List Detailed Ports (GET) - Show Port Details (GET) - Update a Port (PATCH) - Delete Port (DELETE) [1] https://developer.openstack.org/api-ref/baremetal/?expanded=#ports-ports

theopenlab-ci · 2019-02-16T13:20:59Z

Build failed.

gophercloud-unittest : SUCCESS in 2m 32s
gophercloud-acceptance-test : FAILURE in 1h 41m 07s

dtantsur

A few comments inline, but nothing stands out as blocking.

dtantsur · 2019-02-16T14:44:44Z

openstack/baremetal/v1/ports/doc.go

+ 	// Example to Create a Port
+ 	createPort, err := ports.Create(client, ports.CreateOpts{
+ 		NodeUUID: "e8920409-e07e-41bb-8cc1-72acb103e2dd",
+ 		Address: "00-1B-63-84-45-E6",


I think ironic accepts addresses only separated by colon. I may be wrong though, needs double-checking.

yeah I will update

dtantsur · 2019-02-16T14:45:58Z

openstack/baremetal/v1/ports/requests.go

+  Node string `q:"node"`
+
+  // Filter the list by the Node uuid
+  NodeUUID string `q:"node_uuid"`


This is unfortunate, node_uuid is so ancient (API 1.6, Liberty era?), I'd love to avoid it. I wonder if we can default to microversion 1.6 and get rid of this..

Is it not supported on 1.6 or later? It's still listed in the most current API docs

Well, was in the documentation .-.

I think Dmitry might be thinking of /v1/nodes/$uuid/ports/, although that is not how the API is documented.. and I think we should conform to how the API is documented.

dtantsur · 2019-02-16T14:47:40Z

openstack/baremetal/v1/ports/results.go

+	Links []string `json:"links"`
+
+	// Indicates whether the Port is a Smart NIC port.
+	IsSmartnic bool `json:"is_smartnic"`


I wonder if it should be IsSmartNIC, depending on the Go rules.

IsSmartNIC looks best to me

Tks I will update!

stbenjam

To me this looks pretty good, a couple comments below

stbenjam · 2019-02-18T13:08:55Z

openstack/baremetal/v1/ports/requests.go

+// List request.
+type ListOptsBuilder interface {
+	ToPortListQuery() (string, error)
+  ToPortListDetailQuery() (string, error)


Indentation is off here and elsewhere, I would run gofmt on all your files. go is rather opinionated, there's "one" way to format the code, I use this in vim: https://github.com/stbenjam/dotfiles/blob/master/vimrc#L73. GoLand IDE also does it. There's git hooks out there too to run the CLI before commit.

stbenjam · 2019-02-18T13:15:44Z

openstack/baremetal/v1/ports/requests.go

+  Node string `q:"node"`
+
+  // Filter the list by the Node uuid
+  NodeUUID string `q:"node_uuid"`


Is it not supported on 1.6 or later? It's still listed in the most current API docs

stbenjam · 2019-02-18T18:56:59Z

@iurygregory Also can you update the description, so it references #1429 without the brackets so GitHub makes the link automagically?

iurygregory · 2019-02-19T08:32:51Z

@stbenjam going to update

- Fix Address in doc - Renamed IsSmartnic to IsSmartNIC - Fixed Indentation using go fmt - Fixed acceptance test

- Fix acceptance for noauth and v1 to use correct field - Fix undefined field for Port in requests_test

theopenlab-ci · 2019-02-19T11:22:03Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 05s
gophercloud-acceptance-test : FAILURE in 46m 02s

elfosardo · 2019-02-20T10:06:19Z

openstack/baremetal/v1/ports/testing/fixtures.go

+	})
+}
+
+// HandlePortListSuccessfully sets up the test server to respond to a server List request.


nit: should change comment based on the actual function HandlePortListDetailSuccessfully

Tks Riccardo

-Fixed acceptance tests (validated using DevStack - Stein on Ubuntu) -Fixed unnit tests -Fixed Links variable type

- Missing v1 before the functions to create fake node and delete

coveralls · 2019-02-20T14:52:17Z

Coverage increased (+0.2%) to 76.501% when pulling 40ede77 on iurygregory:ironic_port_api into 9e57e2f on gophercloud:master.

theopenlab-ci · 2019-02-20T15:09:20Z

Build failed.

gophercloud-unittest : SUCCESS in 2m 47s
gophercloud-acceptance-test : FAILURE in 50m 24s

jtopjian

Hi all,

I'm not sure if this is ready for an official review yet (when ready, please leave a comment of something like "ready for review"), but figured I would look it over.

First, thank you for the collaborative effort on this. All comments and changes were relevant.

I've left two general comments, possibly requiring no actions.

As I've mentioned before, I'm not familiar with Ironic so I can't comment on the functionality, but the code itself looks good to me. I can merge this when it's ready.

Also, the acceptance test failure can be ignored - I'm in the process of fixing that now.

jtopjian · 2019-02-23T16:47:17Z

openstack/baremetal/v1/ports/requests.go

+	LocalLinkConnection map[string]interface{} `json:"local_link_connection,omitempty"`
+
+	// Indicates whether PXE is enabled or disabled on the Port.
+	PXEEnabled bool `json:"pxe_enabled,omitempty"`


Having bool and omitempty together will never allow a value of false to be sent to the API service. This is because false is bool's zero-value and omitempty omits zero-values.

If it is acceptable for a value of false to never be sent, then this is fine as-is. But if you want the ability to send both true and false, then change this to *bool and keep omitempty.

Alternatively, use bool but remove omitempty to cause false to always be sent as default - but that's rarely the desired behavior.

@jtopjian Thanks i will update.

Updated PXEEnabled and IsSmartNIC

Hmm, the default for pxe_enabled is true, so we must be able to send false.

jtopjian · 2019-02-23T16:48:24Z

openstack/baremetal/v1/ports/requests.go

+type UpdateOperation struct {
+	Op    UpdateOp `json:"op,required"`
+	Path  string   `json:"path,required"`
+	Value string   `json:"value,omitempty"`


A recent issue that was reported is: #1458

I'm not sure if this is applicable here, but wanted to mention it in case.

I will setup a env with Python3 for Ironic and check if this will be a problem

jtopjian

Oops - actually I just spotted two minor changes.

These changes allow the acceptance tests to only run when explicitly triggered.

jtopjian · 2019-02-23T16:56:38Z

acceptance/openstack/baremetal/noauth/ports_test.go

@@ -0,0 +1,71 @@
+package noauth


There needs to be the following two lines before the package statement:

// +build acceptance baremetal ports

The newline is required.

jtopjian · 2019-02-23T16:56:56Z

acceptance/openstack/baremetal/v1/ports_test.go

@@ -0,0 +1,70 @@
+package v1


-Added header for acceptance tests -Change `bool` to `*bool` to allow true and false for PXEEnabled and IsSmartNIC -Change UpdateOperation Value to `interface{}` to avoid problems when not using string (e.g, booleans, jsons)

theopenlab-ci · 2019-02-25T17:00:08Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 04s
gophercloud-acceptance-test : RETRY_LIMIT in 1m 00s

jtopjian · 2019-02-26T04:42:49Z

@iurygregory with the change from bool to *bool, variables will need to be updated to use a reference For example, instead of:

	port, err := ports.Create(client, ports.CreateOpts{
		NodeUUID:   node.UUID,
		Address:    mac,
		PXEEnabled: true,
	}).Extract()

It'll now need to be:

        iTrue := true
	port, err := ports.Create(client, ports.CreateOpts{
		NodeUUID:   node.UUID,
		Address:    mac,
		PXEEnabled: &iTrue,
	}).Extract()

Type `*bool` requires the pointer for a variable with `bool` value

- `itrue` is undefined, change to `iTrue` that is defined on line 71.

theopenlab-ci · 2019-02-26T11:27:15Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 01s
gophercloud-acceptance-test : SUCCESS in 1h 11m 28s

Don't set bool value directly to PXEEnabled since it's `*bool` type

theopenlab-ci · 2019-02-26T13:29:30Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 28s
gophercloud-acceptance-test : SUCCESS in 59m 54s

jtopjian

LGTM

jtopjian · 2019-02-26T16:56:12Z

@iurygregory This looks good to me. Let me know if it's ready to be merged?

iurygregory · 2019-02-26T17:56:28Z

@jtopjian Yeah it's =)

dtantsur approved these changes Feb 16, 2019

View reviewed changes

stbenjam suggested changes Feb 18, 2019

View reviewed changes

stbenjam mentioned this pull request Feb 18, 2019

Add support for Ironic baremetal API #1429

Open

61 tasks

iurygregory added 2 commits February 19, 2019 11:01

Updates for Ports API

3ae7925

- Fix Address in doc - Renamed IsSmartnic to IsSmartNIC - Fixed Indentation using go fmt - Fixed acceptance test

Update Tests

7b2f583

- Fix acceptance for noauth and v1 to use correct field - Fix undefined field for Port in requests_test

elfosardo reviewed Feb 20, 2019

View reviewed changes

iurygregory added 2 commits February 20, 2019 14:54

Update Acceptance and Unit Tests

aa60057

-Fixed acceptance tests (validated using DevStack - Stein on Ubuntu) -Fixed unnit tests -Fixed Links variable type

Fix noauth acceptance

c1c59fb

- Missing v1 before the functions to create fake node and delete

jtopjian reviewed Feb 23, 2019

View reviewed changes

jtopjian requested changes Feb 23, 2019

View reviewed changes

Fixes in requests and acceptance

5e570e9

-Added header for acceptance tests -Change `bool` to `*bool` to allow true and false for PXEEnabled and IsSmartNIC -Change UpdateOperation Value to `interface{}` to avoid problems when not using string (e.g, booleans, jsons)

iurygregory added 2 commits February 26, 2019 10:01

Fix value for type *bool

a5d30be

Type `*bool` requires the pointer for a variable with `bool` value

Fix typo in variable name on acceptance

127e776

- `itrue` is undefined, change to `iTrue` that is defined on line 71.

Fix testing for *bool type

40ede77

Don't set bool value directly to PXEEnabled since it's `*bool` type

jtopjian approved these changes Feb 26, 2019

View reviewed changes

jtopjian merged commit 490361a into gophercloud:master Feb 26, 2019

iurygregory deleted the ironic_port_api branch October 13, 2023 20:49

Conversation

iurygregory commented Feb 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Feb 16, 2019

Uh oh!

dtantsur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stbenjam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stbenjam commented Feb 18, 2019

Uh oh!

iurygregory commented Feb 19, 2019

Uh oh!

theopenlab-ci bot commented Feb 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Feb 20, 2019

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci bot commented Feb 25, 2019

Uh oh!

jtopjian commented Feb 26, 2019

Uh oh!

theopenlab-ci bot commented Feb 26, 2019

iurygregory commented Feb 16, 2019 •

edited

Loading

coveralls commented Feb 20, 2019 •

edited

Loading