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.

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

Use AWS_PROFILE variable for aws named profiles #522

Conversation

RolfKoenders
Copy link
Contributor

@RolfKoenders RolfKoenders commented Oct 5, 2018

Description

Refactor the AWS section to use the correct environment variable that the (new) AWS cli uses. Its using named profiles.

Fixes #259

Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

Thank you @RolfKoenders for the PR 👍

I don't use AWS, But is there any cases where removing AWS_DEFAULT_PROFILE misses valid configurations from prompt ?

It would be better if this change was documented under AWS section.

@salmanulfarzy salmanulfarzy added the improvement A PR that make small changes for improving UX, performance, readability, etc label Oct 6, 2018
@RolfKoenders
Copy link
Contributor Author

Since the AWS_DEFAULT_PROFILE var is completely removed from the awscli i don't expect that to happen. The link in the aws section which links to the documentation of the cli explains this new AWS_PROFILE var.

salmanulfarzy
salmanulfarzy previously approved these changes Oct 6, 2018
Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

Looks like AWS_PROFILE is the recommended solution with backward support for AWS_DEFAULT_PROFILE. Since the changes was introduced more than a year ago in aws-cli, It shouldn't be a problem.

@RolfKoenders
Copy link
Contributor Author

@salmanulfarzy Thanks for updating the documentation! 👍

Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

Code looks good 👍 But we need review from someone who use the section regularly.

@salmanulfarzy salmanulfarzy changed the title Fix AWS named profiles Use AWS_PROFILE variable for aws named profiles Oct 7, 2018
@RolfKoenders
Copy link
Contributor Author

Maybe @cdimitroulas or @robertomoreno can help us out here. ping 😉

@cdimitroulas
Copy link
Contributor

I've been using spaceship with the exact same code change as this PR for a long time and it works great (I know I know... super guilty that I didn't make the PR myself!)

From my perspective this is good to be merged

@salmanulfarzy salmanulfarzy merged commit 720246d into spaceship-prompt:master Oct 9, 2018
@salmanulfarzy
Copy link
Member

Merged, Thank you @RolfKoenders @cdimitroulas 👏

@RolfKoenders RolfKoenders deleted the fix/#259-AWS_named_profiles branch October 9, 2018 06:47
@miller-time miller-time mentioned this pull request Oct 13, 2018
dedene pushed a commit to zenjoy/spaceship-prompt that referenced this pull request Feb 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement A PR that make small changes for improving UX, performance, readability, etc
Development

Successfully merging this pull request may close these issues.

None yet

3 participants