Skip to content

Identity V3: Add access rules support for application credentials#1758

Merged
jtopjian merged 3 commits intogophercloud:masterfrom
kayrus:accessrules
Nov 1, 2019
Merged

Identity V3: Add access rules support for application credentials#1758
jtopjian merged 3 commits intogophercloud:masterfrom
kayrus:accessrules

Conversation

@kayrus
Copy link
Copy Markdown
Contributor

@kayrus kayrus commented Oct 22, 2019

Resolves #1757

acceptance tests will be added later

@jtopjian not sure whether I need to create a separate struct to create access rules

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 22, 2019

Coverage Status

Coverage increased (+0.03%) to 76.973% when pulling ed3821b on kayrus:accessrules into 6ad562a on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 22, 2019

Build succeeded.

@jtopjian
Copy link
Copy Markdown
Contributor

not sure whether I need to create a separate struct to create access rules

Do you mean because you are currently re-using AccessRule from results.go?

Technically, we can't use Links for the creation of an access rule, so that might be reason enough to create a struct in requests.go.

On the other hand, this kind of re-use is done in in a few other places already. In addition, there are a lot of Links fields missing in other structs. To date, no one has reported an issue with this and pagination automatically handles the Links fields when they're missing (just like how you have NextPageURL right now. Here's another example).

So there are two options:

  1. If you don't have a need to use Links, you can remove it from the result struct and re-use that struct for creation, but we'll have to be mindful that this might need changed in the future.

  2. Create an AccessRuleCreateOpts struct that looks like AccessRule but does not include Links.

Thoughts?

@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Oct 29, 2019

recheck

@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Oct 29, 2019

@jtopjian probably the first option is the best choice. I don't have a use case to fetch and process links attribute for an individual access rule and even an application credential.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 29, 2019

Build failed.

@jtopjian
Copy link
Copy Markdown
Contributor

Sorry, I completely forgot about this last week: #1761

I'll try to get to this later today.

@huangtianhua
Copy link
Copy Markdown
Contributor

recheck

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 30, 2019

Build failed.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 30, 2019

Build failed.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 30, 2019

Build succeeded.

@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Oct 30, 2019

@jtopjian ready for review

Copy link
Copy Markdown
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.

@kayrus Sorry for the late review - I was out of town.

Just one small change/revert. Otherwise this looks really good - nice work on the acceptance test.

// The expiration time of the application credential, if one was specified.
ExpiresAt time.Time `json:"-"`
// Links contains referencing links to the application credential.
Links map[string]interface{} `json:"links"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add this back. I mentioned that no one has noticed when these fields are missing, but I'd prefer to leave the existing ones in place just in case they're being used somewhere.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Nov 1, 2019

Build succeeded.

Copy link
Copy Markdown
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.

LGTM - Thank you!

@jtopjian jtopjian merged commit d326b29 into gophercloud:master Nov 1, 2019
@kayrus kayrus deleted the accessrules branch November 2, 2019 07:18
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.

Identity V3: Add access rules field support for application credentials

4 participants