Skip to content

Identity v3: Users Get / List - #288

Merged
jrperritt merged 4 commits into
gophercloud:masterfrom
jtopjian:identityv3-users
Jun 13, 2017
Merged

Identity v3: Users Get / List#288
jrperritt merged 4 commits into
gophercloud:masterfrom
jtopjian:identityv3-users

Conversation

@jtopjian

@jtopjian jtopjian commented Mar 9, 2017

Copy link
Copy Markdown
Contributor

For #287

@jtopjian jtopjian mentioned this pull request Mar 9, 2017
Closed
4 tasks done
@jtopjian jtopjian changed the title Identityv3 users Identity v3: Users Get / List Mar 9, 2017
@coveralls

coveralls commented Mar 9, 2017 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 69.594% when pulling ac67a98 on jtopjian:identityv3-users into b46ba5a on gophercloud:master.

@coveralls

coveralls commented Mar 9, 2017 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 69.594% when pulling 2cf2061 on jtopjian:identityv3-users into b46ba5a on gophercloud:master.

@jtopjian

jtopjian commented Mar 9, 2017

Copy link
Copy Markdown
Contributor Author

@jrperritt Ready for review

@jtopjian

jtopjian commented Mar 10, 2017

Copy link
Copy Markdown
Contributor Author

Found a missing field while working on Create. That's all of them now.

@coveralls

coveralls commented Mar 10, 2017 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 69.589% when pulling 04be3f1 on jtopjian:identityv3-users into b46ba5a on gophercloud:master.

@jtopjian jtopjian changed the title Identity v3: Users Get / List [WIP] Identity v3: Users Get / List Mar 10, 2017
@jtopjian

jtopjian commented Mar 10, 2017

Copy link
Copy Markdown
Contributor Author

Nope. Something is strange here. Making WIP for now.

@coveralls

coveralls commented Mar 10, 2017 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.07%) to 69.637% when pulling 8c6fd3a on jtopjian:identityv3-users into b46ba5a on gophercloud:master.

@jtopjian

jtopjian commented Mar 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@jrperritt Looks like Keystone also supports an arbitrary number of properties like Glance does in #260. There's little documentation on this, but search for "email" here and note this to_dict function.

The second link is noteworthy because it seems Keystone will add them to the general top-level JSON structure and, in some cases, also bundle them in an extra dict.

So while I was trying to avoid #260, I ended up having to solve it anyway.

No doubt the solution I came up with in the latest commit isn't the cleanest way of doing this. Can you take a look when you have time?

@jtopjian jtopjian mentioned this pull request May 4, 2017
Merged
@jtopjian

jtopjian commented May 21, 2017

Copy link
Copy Markdown
Contributor Author

@jrperritt The extra field here is what I was talking about for #194. In this commit, I'm using reflect to bundle the extra fields into the extra field. I'm all thumbs when it comes to reflect, so let me know if there's a better way of doing this.

In the case of this PR, extra is the name of the actual field. #316 added AdditionalProperties to the request body because that was also the name in the API. #260 will be Properties. But when there are cases when we add our own "extra" field, we should probably standardize on a name. Extra?

@jrperritt

jrperritt commented May 24, 2017

Copy link
Copy Markdown
Contributor

let me know if there's a better way of doing this.

I think that way is correct. That functionality would seem appropriate to put in a utility file in the internal package for other Extract function to use.

added AdditionalProperties to the request body because that was also the name in the API

Ah, I didn't realize that.

we should probably standardize on a name. Extra?

Other ideas: Overflow, More, Other.

@jrperritt

jrperritt commented Jun 12, 2017

Copy link
Copy Markdown
Contributor

@jtopjian Was this just waiting on RemainingKeys?

@jtopjian

jtopjian commented Jun 12, 2017

Copy link
Copy Markdown
Contributor Author

@jrperritt Yeah, I'll be working on this this week.

@coveralls

coveralls commented Jun 13, 2017 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.04%) to 70.663% when pulling 04cbe18 on jtopjian:identityv3-users into 64e5161 on gophercloud:master.

@jtopjian jtopjian changed the title [WIP] Identity v3: Users Get / List Identity v3: Users Get / List Jun 13, 2017
@jtopjian
jtopjian requested a review from jrperritt June 13, 2017 03:00
@jtopjian

jtopjian commented Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

@jrperritt This is ready for review.

I'm not sure what happened, but somewhere along the way my branch got mangled. Since I had to rebase, I figure I'd squash the commits.

I reviewed everything today and even found a new user attribute of Options. The links in #287 have been updated, too.

@coveralls

coveralls commented Jun 13, 2017 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.04%) to 70.663% when pulling 6c1b72f on jtopjian:identityv3-users into 64e5161 on gophercloud:master.

jrperritt
jrperritt suggested changes Jun 13, 2017
View reviewed changes
Comment thread openstack/identity/v3/users/requests.go Outdated
Enabled *bool `q:"enabled"`

// IdpID filters the response by an Identity Provider ID.
IdPID string `q:"enabled"`

@jrperritt jrperritt Jun 13, 2017

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.

q:"?"

Comment thread openstack/identity/v3/users/requests.go Outdated
}

// GetOpts allows you to modify the details included in the Get request.
type GetOpts struct{}

@jrperritt jrperritt Jun 13, 2017

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.

What are the options here?

@jtopjian jtopjian Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There aren't any. This is only to satisfy the ToUserGetQuery method for possible extension. Should this be removed altogether?

@jrperritt jrperritt Jun 13, 2017

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.

I don't think any of the other services offer this. I'm trying to think of what options could even by provided considering the ID uniquely identifies the user.

@jtopjian jtopjian Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree. I did the same for Projects, but in hindsight, I have no idea why. I must have had a reason or known of some customization in the wild.

I'm OK with removing this and adding it later if it comes up.

@jrperritt

jrperritt commented Jun 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

This one looks good to me. We'll probably want to remove the GetOptsBuilder from v3/projects in a separate PR.

@jrperritt
jrperritt merged commit 15998c6 into gophercloud:master Jun 13, 2017
@jtopjian

jtopjian commented Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

Thanks!

I'll submit a PR for v3/projects now while it's on my mind.

@jtopjian jtopjian mentioned this pull request Jun 13, 2017
Closed
@coveralls

coveralls commented Jun 13, 2017 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 70.726% when pulling bc289d6 on jtopjian:identityv3-users into 64e5161 on gophercloud:master.

@jtopjian jtopjian mentioned this pull request Jun 14, 2017
Merged
j-griffith pushed a commit to j-griffith/gophercloud that referenced this pull request Jul 15, 2017
@jtopjian j-griffith
Identity v3: Users Get / List (gophercloud#288)
6d273ed 
* Identity v3: Users Get / List

* Identity v3: Users Get / List unit tests

* Identity v3: Users Get / List acceptance tests

* Identity v3: Users code review updates
@jtopjian
jtopjian deleted the identityv3-users branch April 24, 2018 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@jrperritt jrperritt jrperritt 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.

3 participants

@jtopjian @coveralls @jrperritt