-
Notifications
You must be signed in to change notification settings - Fork 88
CLOUDP-120254: Address advocate experience feedback #1281
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.
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
Conversation
andreaangiolillo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
melissamahoney-mongodb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions to make it a little shorter and cleaner. :)
internal/cli/auth/login.go
Outdated
| // Only make references to profile if user was asked about org or projects | ||
| if opts.AskedOrgsOrProjects && opts.ProjectID != "" && opts.OrgID != "" { | ||
| if !opts.ProjectExists(config.ProjectID()) { | ||
| return fmt.Errorf("you don't have access to the project id you provided or it does not exist") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return fmt.Errorf("you don't have access to the project id you provided or it does not exist") | |
| return fmt.Errorf("You don't have access to this project ID or it doesn't exist.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, addressed most of them except the upper case and the period because for error messages we cannot add this (linter doesn't allow)
internal/cli/auth/login.go
Outdated
| } | ||
|
|
||
| if !opts.OrgExists(config.OrgID()) { | ||
| return fmt.Errorf("you don't have access to the organization id you provided or it does not exist") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return fmt.Errorf("you don't have access to the organization id you provided or it does not exist") | |
| return fmt.Errorf("You don't have access to this organization ID or it doesn't exist.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, addressed most of them except the upper case and the period because for error messages we cannot add this (linter doesn't allow)
|
@blva thanks for the changes |
* CLOUDP-120254: Address advocate experience feedback (#1281) * CLOUDP-124560: Add login to quickstart (#1284) * CLOUDP-118914: Clean up after tests that set config (#1285) * CLOUDP-118914: Save selected values and ask setup in a loop (#1297) * license header Co-authored-by: Bianca <40155621+blva@users.noreply.github.com> Co-authored-by: Bianca <bianca.vianadeaguiar@mongodb.com>
Proposed changes
Jira ticket:CLOUDP-120254
Improve the flow experience when user types invalid project id. Previously we were setting the profile and later failing with
Closes #[issue number]
Checklist
make fmtand formatted my codeFurther comments