Skip to content

CLOUDP-58790: Take database name from a flag when handling dbusers, Atlas #53

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
Merged
merged 11 commits into from
Mar 19, 2020
Merged

CLOUDP-58790: Take database name from a flag when handling dbusers, Atlas #53

merged 11 commits into from
Mar 19, 2020

Conversation

andreaangiolillo
Copy link
Collaborator

Jira ticket: CLOUDP-58790

  • 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

@andreaangiolillo andreaangiolillo added the WIP Work in progress label Mar 19, 2020
andreaangiolillo
andreaangiolillo commented Mar 19, 2020
View reviewed changes
internal/cli/cli.go
@@ -60,11 +60,15 @@ type deleteOpts struct {
confirm bool
successMessage string
failMessage string
authDB string
}

// DeleterFromProject a function to delete from the store.
type DeleterFromProject func(projectID string, entry string) error
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure about why we are doing all of this before calling the delete function in the store

andreaangiolillo
andreaangiolillo commented Mar 19, 2020
View reviewed changes
internal/cli/cli.go
@@ -83,6 +87,24 @@ func (opts *deleteOpts) DeleteFromProject(d DeleterFromProject, projectID string
return nil
}

// DeleterFromProjectAuthDB deletes a resource from a project, it expects a callback
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not why after this change every time I run a test regarding any delete I get === RUN TestAtlasDBUsersDelete_Run %!(EXTRA string=test)--- PASS: TestAtlasDBUsersDelete_Run (0.00s) PASS.

There is this extra string (EXTRA string=test), and I do not know where it came from. I also tried to revert all of my changes but I still get the same result 🤕

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed the issue by adding the successMessage to the test. The problem was caused by fmt.Printf(opts.successMessage, opts.entry) inside the function DeleterFromProject which was executed without the successMessage

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we improve on the deleteOpts api then? let's do this in another PR but this was probably an overlook from my side, we could improve this by having a fallback message and check the value we are using, like a said separate PR to not complicate this too much

andreaangiolillo
andreaangiolillo commented Mar 19, 2020
View reviewed changes
internal/usage/usage.go Outdated
@@ -23,6 +23,7 @@ const (
DiskSizeGB = "Capacity, in gigabytes, of the host’s root volume."
Backup = "If true, uses Atlas Continuous Backups to back up cluster data."
MDBVersion = "MongoDB version of the cluster to deploy."
AuthDB = "Database name."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure about the purpose of this new flag, this is why I put this poor description

Copy link
Collaborator

@gssbzn gssbzn Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

let's use "Authentication database name."

@andreaangiolillo andreaangiolillo requested a review from gssbzn March 19, 2020 15:50
@andreaangiolillo andreaangiolillo removed the WIP Work in progress label Mar 19, 2020
gssbzn
gssbzn requested changes Mar 19, 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 but looking good in general

internal/cli/atlas_dbusers_delete.go Outdated
@@ -70,6 +70,7 @@ func AtlasDBUsersDeleteBuilder() *cobra.Command {
cmd.Flags().BoolVar(&opts.confirm, flags.Force, false, usage.Force)

cmd.Flags().StringVar(&opts.projectID, flags.ProjectID, "", usage.ProjectID)
cmd.Flags().StringVar(&opts.authDB, flags.AuthDB, "", usage.AuthDB)
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's make this retro-compatible, this means we don't require it and we default to "admin"

Suggested change
cmd.Flags().StringVar(&opts.authDB, flags.AuthDB, "", usage.AuthDB)
cmd.Flags().StringVar(&opts.authDB, flags.AuthDB, "admin", usage.AuthDB)

internal/cli/cli.go
@@ -83,6 +87,24 @@ func (opts *deleteOpts) DeleteFromProject(d DeleterFromProject, projectID string
return nil
}

// DeleterFromProjectAuthDB deletes a resource from a project, it expects a callback
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we improve on the deleteOpts api then? let's do this in another PR but this was probably an overlook from my side, we could improve this by having a fallback message and check the value we are using, like a said separate PR to not complicate this too much

internal/cli/cli.go Outdated
@@ -60,11 +60,15 @@ type deleteOpts struct {
confirm bool
successMessage string
failMessage string
authDB string
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should not be here, not all deleters should have an authDB, this should work the same as a project id that is passed as a parameter of the function

internal/cli/cli.go Outdated
fmt.Println(opts.failMessage)
return nil
}
err := d(opts.authDB, projectID, opts.entry)
Copy link
Collaborator

Choose a reason for hiding this comment

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

like I mentioned before we should pass authDB as a parameter and not as a member of opts

@andreaangiolillo andreaangiolillo requested a review from gssbzn March 19, 2020 16:30
gssbzn
gssbzn approved these changes Mar 19, 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.

LGTM great work here

@andreaangiolillo andreaangiolillo merged commit a2698ab into master Mar 19, 2020
@andreaangiolillo andreaangiolillo deleted the CLOUDP-58790 branch March 19, 2020 16:38
corryroot pushed a commit to corryroot/mongodb-atlas-cli that referenced this pull request Jun 15, 2023
@sarahsimpers
(DOCSP-23270)(DOCSP-23232) Release notes, source constant updates for…
22b3600 
… v1.1.4 and telemetry changes (mongodb#53)

* (DOCSP-23270)(DOCSP-23232) Release notes, source constant updates for v1.1.4 and telemetry changes

* Sets submodule to atlascli/v1.1.4

* Line break change from copy review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.
2 participants
@andreaangiolillo @gssbzn