Skip to content

Compute v2: Flavor Access Add#687

Merged
jtopjian merged 2 commits intogophercloud:masterfrom
jtopjian:computev2-flavor-access-add
Dec 21, 2017
Merged

Compute v2: Flavor Access Add#687
jtopjian merged 2 commits intogophercloud:masterfrom
jtopjian:computev2-flavor-access-add

Conversation

@jtopjian
Copy link
Copy Markdown
Contributor

@jtopjian jtopjian commented Dec 21, 2017

For #506

Code and reference is listed in #506

@jtopjian
Copy link
Copy Markdown
Contributor Author

jtopjian commented Dec 21, 2017

I forgot I had this ready to go. Rebased and cleaned up.

Ready for review.

@jtopjian jtopjian mentioned this pull request Dec 21, 2017
Closed
3 tasks
@coveralls
Copy link
Copy Markdown

coveralls commented Dec 21, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 72.854% when pulling 2c7ec3b on jtopjian:computev2-flavor-access-add into cf81d92 on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 21, 2017

Build failed.

@jtopjian
Copy link
Copy Markdown
Contributor Author

jtopjian commented Dec 21, 2017

Ah, OpenLab was helpful here:

compute.go:277: Attempting to create flavor flavor_o8Ozf
compute.go:293: Successfully created flavor b98d6b36-9772-4cf0-a414-b9e3a452326e
flavors_test.go:137: Unable to add access to flavor: Invalid request due to incorrect syntax or missing required parameters.
compute.go:593: Deleted flavor: b98d6b36-9772-4cf0-a414-b9e3a452326e

The test passes for me in Ocata. I wonder if Pike now validates the tenant ID.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 21, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 72.854% when pulling 2af216c on jtopjian:computev2-flavor-access-add into cf81d92 on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 21, 2017

Build succeeded.

@jtopjian
Copy link
Copy Markdown
Contributor Author

jtopjian commented Dec 21, 2017

Yep. Hey, look at that. CI in action 😄

dklyle
dklyle suggested changes Dec 21, 2017
View reviewed changes
openstack/compute/v2/flavors/doc.go
flavorID := "e91758d6-a54a-4778-ad72-0c73a1cb695b"

accessOpts := flavors.AddAccessOpts{
Tenant: "15153a0979884b59b0592248ef947921",
Copy link
Copy Markdown
Contributor

@dklyle dklyle Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Since this is the documentation, shouldn't this be an UUID to provide the additional visual prompt as to an appropriate value? I know in the AddAccessOpts definition it states project/tenant ID, but I think it would be helpful to change here. It's an easy change, but not necessarily enough to hold up the patch on.

Copy link
Copy Markdown
Contributor

@dklyle dklyle Dec 21, 2017

Choose a reason for hiding this comment

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

The rest of the patch looks great.

Copy link
Copy Markdown
Contributor Author

@jtopjian jtopjian Dec 21, 2017

Choose a reason for hiding this comment

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

At least on Ocata, the project and user IDs are these kinds of strings rather than UUIDs:

Successfully created project ACPTTESTWZC1JOxR with ID 8befa1ba8c224584bb2b7b55c6e35a24

Has this changed post-Ocata?

Copy link
Copy Markdown
Contributor

@dklyle dklyle Dec 21, 2017

Choose a reason for hiding this comment

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

no, you're right. sorry for the noise.

Copy link
Copy Markdown
Contributor Author

@jtopjian jtopjian Dec 21, 2017

Choose a reason for hiding this comment

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

It's no problem at all.

dklyle
dklyle approved these changes Dec 21, 2017
View reviewed changes
openstack/compute/v2/flavors/doc.go
flavorID := "e91758d6-a54a-4778-ad72-0c73a1cb695b"

accessOpts := flavors.AddAccessOpts{
Tenant: "15153a0979884b59b0592248ef947921",
Copy link
Copy Markdown
Contributor

@dklyle dklyle Dec 21, 2017

Choose a reason for hiding this comment

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

no, you're right. sorry for the noise.

@jtopjian
Copy link
Copy Markdown
Contributor Author

jtopjian commented Dec 21, 2017

@dklyle thank you for the review.

I'm going to go ahead and merge this. It's a pretty standard addition and between @dklyle's review and the OL CI catching an error, I think we're good.

@jtopjian jtopjian merged commit 7b1b877 into gophercloud:master Dec 21, 2017
jrperritt
jrperritt reviewed Dec 21, 2017
View reviewed changes
openstack/compute/v2/flavors/requests.go
// AddAccessOptsBuilder allows extensions to add additional parameters to the
// AddAccess requests.
type AddAccessOptsBuilder interface {
ToAddAccessMap() (map[string]interface{}, error)
Copy link
Copy Markdown
Contributor

@jrperritt jrperritt Dec 21, 2017

Choose a reason for hiding this comment

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

@jtopjian I think these always need to be of the form To<Resource><Action><Return Type> (i.e. ToFlavorAddAccessMap)

Copy link
Copy Markdown
Contributor Author

@jtopjian jtopjian Dec 21, 2017

Choose a reason for hiding this comment

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

Yes, you're right. I've fixed this in #688. Sorry about that.

@jtopjian jtopjian deleted the computev2-flavor-access-add branch April 24, 2018 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@jrperritt jrperritt jrperritt left review comments

@dklyle dklyle dklyle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jtopjian @coveralls @jrperritt @dklyle