Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CLOUDP-70010: iam users describe #384

Merged
merged 33 commits into from
Sep 1, 2020
Merged

CLOUDP-70010: iam users describe #384

merged 33 commits into from
Sep 1, 2020

Conversation

robcarlan-mlab
Copy link
Contributor

@robcarlan-mlab robcarlan-mlab commented Aug 26, 2020 •
edited by andreaangiolillo

Proposed changes

Jira ticket: CLOUDP-70010

Checklist

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Further comments

Usage:
  mongocli iam users describe [flags]

Aliases:
  describe, get

Examples:

  Describe a user by ID
  $ mongocli iam users describe --id <id>

  Describe a user by username
  $ mongocli iam users describe --username <username>

OM:

./bin/mongocli iam users describe --username rob.carlan@mongodb.com --profile om
ID                         FIRST NAME   LAST NAME   USERNAME
5f4579d04bba012f25a03212   rob          carlan      rob.carlan@mongodb.com

Atlas:

./bin/mongocli iam users describe --username rob.carlan@mongodb.com
ID                         FIRST NAME   LAST NAME   USERNAME
5c80f75fcf09a246878f67a4   Rob          Carlan      rob.carlan@mongodb.com

@robcarlan-mlab
add users list
39071d4
@robcarlan-mlab
fix client usage
3ca5ef7
@robcarlan-mlab
add IAM user describe
d5bc60f
@robcarlan-mlab
Merge branch 'master' into CLOUDP-70013-iam-users-list
aeb026e
@robcarlan-mlab
Merge branch 'master' into CLOUDP-700100-iam-users-describe
f64a3ff
@robcarlan-mlab
remove local development line
92d1258
@robcarlan-mlab
Merge branch 'CLOUDP-70013-iam-users-list' of github.com:mongodb/mong…
c5a5c56 
…ocli into CLOUDP-70013-iam-users-list
@robcarlan-mlab
Merge branch 'CLOUDP-70013-iam-users-list' into CLOUDP-700100-iam-use…
b380370 
…rs-describe
@robcarlan-mlab
update OM client version
f993e7e
@robcarlan-mlab
undo isatty upgrade
abfc5ba
@robcarlan-mlab
Merge branch 'master' into CLOUDP-70013-iam-users-list
72ecba1
@robcarlan-mlab
add e2e
7190c46
@robcarlan-mlab
Merge branch 'CLOUDP-70013-iam-users-list' of github.com:mongodb/mong…
b6c37fd 
…ocli into CLOUDP-70013-iam-users-list
@robcarlan-mlab
add e2e
3694cb4
@robcarlan-mlab
add e2e
2e4e2b4
@robcarlan-mlab
Merge branch 'master' into CLOUDP-70013-iam-users-list
a58320a
@robcarlan-mlab
preemptively fix e2e
ba6eee3
@robcarlan-mlab
Merge branch 'CLOUDP-70013-iam-users-list' of github.com:mongodb/mong…
07a16ee 
…ocli into CLOUDP-70013-iam-users-list
@robcarlan-mlab
Merge branch 'CLOUDP-70013-iam-users-list' into CLOUDP-700100-iam-use…
abc1a80 
…rs-describe
@robcarlan-mlab
e2e test fix
e774b2b
@robcarlan-mlab
Merge branch 'master' into CLOUDP-700100-iam-users-describe
0eb249f
gssbzn
gssbzn requested changes Sep 1, 2020
View reviewed changes
Copy link
Collaborator

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

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

Couple of nits here and there

internal/cli/iam/users/describe.go Outdated Show resolved Hide resolved
internal/cli/iam/users/describe.go Outdated Show resolved Hide resolved
internal/cli/iam/users/describe.go Outdated Show resolved Hide resolved
internal/cli/iam/users/describe.go Outdated Show resolved Hide resolved
internal/cli/iam/users/describe.go Outdated Show resolved Hide resolved
internal/cli/iam/users/description.go Outdated Show resolved Hide resolved
internal/cli/iam/users/users.go Outdated Show resolved Hide resolved
internal/store/users.go Outdated Show resolved Hide resolved
e2e/atlas/users_test.go Outdated Show resolved Hide resolved
gssbzn
gssbzn requested changes Sep 1, 2020
View reviewed changes
Copy link
Collaborator

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

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

One big thing I just caught, we may be forcing users to have a ProjectID set for this command

internal/cli/iam/users/describe.go Outdated Show resolved Hide resolved
internal/cli/iam/users/describe.go Outdated Show resolved Hide resolved
internal/cli/iam/users/describe.go Outdated Show resolved Hide resolved
internal/cli/iam/users/describe.go Outdated Show resolved Hide resolved
@andreaangiolillo
Copy link
Collaborator

@gssbzn ready for review

gssbzn
gssbzn approved these changes Sep 1, 2020
View reviewed changes
Copy link
Collaborator

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

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

One small nit with the test but we can address that later

e2e/iam/atlas_users_test.go
Comment on lines +80 to +82
if err := json.Unmarshal(resp, &user); err != nil {
t.Fatalf("unexpected error: %v", err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] can you please check that the username matches the one you asked for?

e2e/iam/atlas_users_test.go
Comment on lines +102 to +104
if err := json.Unmarshal(resp, &user); err != nil {
t.Fatalf("unexpected error: %v", err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] similar to above, right now we only check it doesn't error but we should also check the data, at least the id matches

@andreaangiolillo andreaangiolillo merged commit fbe27f0 into master Sep 1, 2020
@andreaangiolillo andreaangiolillo deleted the CLOUDP-700100-iam-users-describe branch September 1, 2020 15:41
@andreaangiolillo
Copy link
Collaborator

I will address those comments here #389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreaangiolillo andreaangiolillo andreaangiolillo left review comments

@gssbzn gssbzn gssbzn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@robcarlan-mlab @andreaangiolillo @gssbzn