Skip to content

chore: update recommended actions for missing credentials #4089

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

Merged
mergify merged 1 commit into from
Oct 14, 2022
Merged

chore: update recommended actions for missing credentials #4089

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions internal/pkg/aws/sessions/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,11 @@ func (e *errCredRetrieval) Error() string {
// RecommendActions returns recommended actions to be taken after the error.
// Implements main.actionRecommender interface.
func (e *errCredRetrieval) RecommendActions() string {
notice := "It looks like your credential settings are misconfigured or missing"
notice := "It looks like AWS credentials are misconfigured or missing"
Copy link
Copy Markdown
Contributor

@huanjani huanjani Oct 14, 2022

Choose a reason for hiding this comment

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

Suggested change
notice := "It looks like AWS credentials are misconfigured or missing"
notice := "It looks like your AWS credentials are misconfigured or missing"

Copy link
Copy Markdown
Contributor Author

@genbit genbit Oct 14, 2022

Choose a reason for hiding this comment

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

curious, why your is needed?

Copy link
Copy Markdown
Contributor

@huanjani huanjani Oct 14, 2022

Choose a reason for hiding this comment

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

So the user knows it's their issue to fix rather than something internal.

if e.profile != "" {
notice = fmt.Sprintf("It looks like your profile [%s] is misconfigured or missing", e.profile)
notice = fmt.Sprintf("It looks like AWS profile [%s] is misconfigured or missing", e.profile)
Copy link
Copy Markdown
Contributor

@huanjani huanjani Oct 14, 2022

Choose a reason for hiding this comment

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

Suggested change
notice = fmt.Sprintf("It looks like AWS profile [%s] is misconfigured or missing", e.profile)
notice = fmt.Sprintf("It looks like your AWS profile [%s] is misconfigured or missing", e.profile)

Copy link
Copy Markdown
Contributor Author

@genbit genbit Oct 14, 2022

Choose a reason for hiding this comment

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

curious, why your is needed?

Copy link
Copy Markdown
Contributor

@huanjani huanjani Oct 14, 2022

Choose a reason for hiding this comment

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

This one not so much bc you name the profile. So we could take it or leave it. But it emphasizes that it's the user's profile.

}
return fmt.Sprintf(`%s:
https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/configuring-sdk.html#specifying-credentials
- We recommend including your credentials in the shared credentials file.
- Alternatively, you can also set credentials through
* Environment Variables
* EC2 Instance Metadata (credentials only)
Comment on lines -47 to -51
Copy link
Copy Markdown
Contributor

@iamhopaul123 iamhopaul123 Oct 14, 2022

Choose a reason for hiding this comment

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

Why do we remove these 🤔

Copy link
Copy Markdown
Contributor Author

@genbit genbit Oct 14, 2022

Choose a reason for hiding this comment

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

Documentation link has ll details and for someone who is using copilot for first time and doesnt have relevant experience of dealing with aws from terminal — those don’t mean match without context. In fact could give a wrong impression that one need to run copilot from within EC2 instance.

If the goal for those messages to provide ideas on how someone who think they configured credentials and now need to debug missing credentials error - we should think more about this use case and how to help

Copy link
Copy Markdown
Contributor

@iamhopaul123 iamhopaul123 Oct 14, 2022

Choose a reason for hiding this comment

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

Gotcha makes sense to me.

More information: https://aws.github.io/copilot-cli/docs/credentials/`, notice)
}

Expand Down