Add the user id to the get create and delete keypair by randomswdev · Pull Request #2146 · gophercloud/gophercloud

randomswdev · 2021-04-25T20:43:25Z

The capability to create of get the keypair of another user is documented in the Nova Rest API reference:
https://docs.openstack.org/api-ref/compute/?expanded=list-keypairs-detail,show-keypair-details-detail,create-or-import-keypair-detail#create-or-import-keypair

This pull request adds the capability to create or get the keypairs also for another user.

coveralls · 2021-04-25T20:56:21Z

Coverage increased (+0.008%) to 79.924% when pulling 9815af9 on randomswdev:keypair-add-userid into 05c2cc8 on gophercloud:master.

theopenlab-ci · 2021-04-25T21:15:24Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 31s
gophercloud-acceptance-test : POST_FAILURE in 24m 32s
gophercloud-acceptance-test-ironic : SUCCESS in 23m 59s

randomswdev · 2021-04-26T07:23:31Z

I think the build failure is not related to my ciode, but is more of an environmental issue. Can someone help me fixing it?

jtopjian · 2021-04-26T14:24:53Z

@randomswdev Thanks for submitting this. I encourage you to read our contributor tutorial, specifically Step 3, to gather all of the information needed for this PR.

The way this is being implemented is a pretty large breaking change. Normally I don't mind merging patches that cause breaking changes, key pairs are a core resource of openstack and this PR would require a lot of downstream code to be rewritten. Additionally, since this feature was added in a microversion, it's technically not part of the base API calls.

Adding UserID to CreateOpts is fine to do, as long as the guidelines here are followed.

For the Get and Delete functions, I suggest creating new functions for Get and Delete that specifically pass the userID field. Perhaps GetWithUserID and DeleteWithUserID.

Please let me know if you have any questions.

randomswdev · 2021-04-26T20:13:37Z

@jtopjian thanks for the comments.

Adding UserID to CreateOpts is fine to do, as long as the guidelines here are followed.

The UserID field is flagged as omitempty, so should be ok based on the guidelines.

For the Get and Delete functions, I suggest creating new functions for Get and Delete that specifically pass the userID field. Perhaps GetWithUserID and DeleteWithUserID.

I added the GetWithUserID and DeleteWithUserID functions, and reverted the changes to the interface of Get and Delete, that are now backward compatible.

Let me know if other changes are needed. I'm monuitoring the openlab/check status: if it fails again, is there anything I can do to fix it?

jtopjian · 2021-04-26T20:35:05Z

@randomswdev Thanks!

The OpenLab error might be temporary. If it fails again, you can retrigger it by adding a comment with the single word "recheck".

theopenlab-ci · 2021-04-26T21:18:36Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 52s
gophercloud-acceptance-test : POST_FAILURE in 25m 37s
gophercloud-acceptance-test-ironic : SUCCESS in 48m 42s

jtopjian · 2021-04-27T16:28:50Z

recheck

theopenlab-ci · 2021-04-27T16:54:07Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 48s
gophercloud-acceptance-test : POST_FAILURE in 19m 19s
gophercloud-acceptance-test-ironic : SUCCESS in 24m 13s

jtopjian · 2021-04-28T01:18:51Z

@randomswdev The OpenLab error is ignorable - you aren't modifying any of the existing acceptance tests, so it's a problem with OpenLab itself.

Let me know if you're done working on this and if it's ready to be merged.

randomswdev · 2021-06-09T13:06:23Z

Let me know if you're done working on this and if it's ready to be merged.

@jtopjian sorry for the delay in answering. Yes, I'm done with hte implementation, so if it is ok for you I think it can be merged.

randomswdev · 2021-06-09T13:06:37Z

recheck

theopenlab-ci · 2021-06-09T13:20:40Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 27s
gophercloud-acceptance-test : FAILURE in 7m 59s
gophercloud-acceptance-test-ironic : FAILURE in 7m 18s

randomswdev · 2021-06-09T14:11:21Z

Just rebased on the latest master.

theopenlab-ci · 2021-06-09T14:25:47Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 33s
gophercloud-acceptance-test : FAILURE in 13m 28s
gophercloud-acceptance-test-ironic : FAILURE in 12m 52s

jtopjian

@randomswdev Thanks again for working on this. I've left some comments - please let me know if you have any questions or need any help.

jtopjian · 2021-06-10T20:09:44Z

openstack/compute/v2/extensions/keypairs/requests.go

 // Get returns public data about a previously uploaded KeyPair.
 func Get(client *gophercloud.ServiceClient, name string) (r GetResult) {
-	resp, err := client.Get(getURL(client, name), &r.Body, nil)
+	r = GetWithUserID(client, name, "")


Get should not be coupled to the new GetWithUserID. They need to be two separate functions that can work unrelated.

jtopjian · 2021-06-10T20:10:13Z

openstack/compute/v2/extensions/keypairs/requests.go

 // Delete requests the deletion of a previous stored KeyPair from the server.
 func Delete(client *gophercloud.ServiceClient, name string) (r DeleteResult) {
-	resp, err := client.Delete(deleteURL(client, name), nil)
+	r = DeleteWithUserID(client, name, "")


Similar here, Delete should not call DeleteWithUserID.

jtopjian · 2021-06-10T20:12:06Z

openstack/compute/v2/extensions/keypairs/requests.go

+
+// GetWithUserID returns public data about a previously uploaded KeyPair, owned by a specific userID.
+// If the userID is an empty string, it searches the KeyPairs of the current user.
+func GetWithUserID(client *gophercloud.ServiceClient, name string, userID string) (r GetResult) {


Instead of passing in userID in the function parameters, create a GetOpts struct that contains a UserID field. See here for an example: https://github.com/gophercloud/gophercloud/blob/master/openstack/compute/v2/extensions/limits/requests.go

The reason this is done is because the user ID is a query parameter of the URL and not part of the URL itself.

jtopjian · 2021-06-10T20:14:07Z

openstack/compute/v2/extensions/keypairs/requests.go

+
+// DeleteWithUserID requests the deletion of a previous stored KeyPair from the server, owned by a specific userID.
+// If the userID is an empty string, it deletes the KeyPair of the calling user.
+func DeleteWithUserID(client *gophercloud.ServiceClient, name string, userID string) (r DeleteResult) {


Similar here: there needs to be a DeleteOpts struct created: https://github.com/gophercloud/gophercloud/blob/master/openstack/blockstorage/v3/qos/requests.go#L67-L100

jtopjian · 2021-06-10T20:14:53Z

openstack/compute/v2/extensions/keypairs/urls.go


-func getURL(c *gophercloud.ServiceClient, name string) string {
-	return c.ServiceURL(resourcePath, name)
+func getURL(c *gophercloud.ServiceClient, name string, userID string) string {


By using a GetOpts struct, you should be able to revert getURL back to the original code and just append the query params.

jtopjian · 2021-06-10T20:15:10Z

openstack/compute/v2/extensions/keypairs/urls.go


-func deleteURL(c *gophercloud.ServiceClient, name string) string {
-	return getURL(c, name)
+func deleteURL(c *gophercloud.ServiceClient, name string, userID string) string {


Similar here: by using DeleteOpts, you should be able to revert this.

jtopjian · 2021-07-02T13:33:34Z

@randomswdev Thank you for your work on this. This work was amended to #2186 and merged.

randomswdev mentioned this pull request Apr 26, 2021

Add the user_id key to the keypair resource and data source terraform-provider-openstack/terraform-provider-openstack#1233

Closed

randomswdev force-pushed the keypair-add-userid branch from 5294dc7 to e0e4bc4 Compare April 26, 2021 20:27

randomswdev added 3 commits June 9, 2021 16:09

Add the user id to the get create and delete keypair

96a0149

Fix the build

ef89d3c

Code review changes

9815af9

randomswdev force-pushed the keypair-add-userid branch from e0e4bc4 to 9815af9 Compare June 9, 2021 14:10

jtopjian requested changes Jun 10, 2021

View reviewed changes

jtopjian mentioned this pull request Jun 30, 2021

Compute v2: Add the user id to the get create and delete keypair #2186

Merged

jtopjian closed this Jul 2, 2021

Conversation

randomswdev commented Apr 25, 2021 • edited by jtopjian Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Apr 25, 2021

Uh oh!

randomswdev commented Apr 26, 2021

Uh oh!

jtopjian commented Apr 26, 2021

Uh oh!

randomswdev commented Apr 26, 2021

Uh oh!

jtopjian commented Apr 26, 2021

Uh oh!

theopenlab-ci bot commented Apr 26, 2021

Uh oh!

jtopjian commented Apr 27, 2021

Uh oh!

theopenlab-ci bot commented Apr 27, 2021

Uh oh!

jtopjian commented Apr 28, 2021

Uh oh!

randomswdev commented Jun 9, 2021

Uh oh!

randomswdev commented Jun 9, 2021

Uh oh!

theopenlab-ci bot commented Jun 9, 2021

Uh oh!

randomswdev commented Jun 9, 2021

Uh oh!

theopenlab-ci bot commented Jun 9, 2021

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

jtopjian Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

jtopjian Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

jtopjian Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

jtopjian Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

jtopjian Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

jtopjian Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

jtopjian commented Jul 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

randomswdev commented Apr 25, 2021 •

edited by jtopjian

Loading

coveralls commented Apr 25, 2021 •

edited

Loading