Add support for AWS_PROFILE and AWS_DEFAULT_PROFILE environment variables #496

sagepe · 2016-12-06T20:48:48Z

This is useful when working with multiple AWS accounts over the API or CLI. This simple addition will ensure AWS_DEFAULT_PROFILE is displayed if set.

dolmen · 2017-02-10T17:15:55Z

I'm sorry but LiquidPrompt implementation is not flexible enough to allow to support any additional development environment variable without adding a runtime cost.

sagepe · 2017-02-14T17:19:04Z

Thanks for taking the time to review and comment, @dolmen. It's something I need and use so had added to my fork and thought I'd at least send over a PR in case it was of interest.

Rycieos · 2021-01-28T19:56:59Z

Release Candidate v2.0.0-rc.1 is now out, which means that the rework is complete, which means the runtime cost of new features is greatly reduced! I would merge this, but first this has merge conflicts that need to be fixed.

If you need some guidance around the new architecture, see commit f35d9ac for a good example of the things you will need to change. And feel free to ask for help!

sagepe · 2021-01-29T11:45:59Z

How's this?

Rycieos

A few bugs, a few small nitpicks, and a few documentation clarifications.

This in general looks good, other than those fixes.

FYI, the merge window for v2.0 is closed (now just waiting on bug reports), so I can't merge this until v2.0 is fully released. Which means your versionadded lines need to say 2.1. And there were a few places in the documentation that you missed adding a .. versionadded:: block, double check those.

Rycieos · 2021-01-29T15:35:59Z

docs/functions/theme.rst

@@ -90,6 +90,12 @@ specific text and formatting may change.

   .. versionadded:: 2.0

+.. function:: _lp_aws_profile() -> var:lp_aws_profile


Shouldn't this be _lp_awp_profile_color()?

Suggested change

.. function:: _lp_aws_profile() -> var:lp_aws_profile

.. function:: _lp_aws_profile_color() -> var:lp_aws_profile_color

Yes, quite right. Fixed.

Rycieos · 2021-01-29T15:37:58Z

liquidprompt

+        __lp_escape "${AWS_PROFILE##*/}"
+        lp_aws_profile=$ret
+    elif [[ -n "${AWS_DEFAULT_PROFILE-}" ]]; then
+        __lp_escape "${AWS_PROFILE##*/}"


This should return AWS_DEFAULT_PROFILE

Suggested change

__lp_escape "${AWS_PROFILE##*/}"

__lp_escape "${AWS_DEFAULT_PROFILE##*/}"

Not needed following the simplification you suggest below.

Rycieos · 2021-01-29T16:35:44Z

docs/config.rst

+   :type: bool
+   :value: 1
+
+   Display the current value of the ``AWS_PROFILE`` or ``AWS_DEFAULT_PROFILE``


Prefix these with :envvar:, like this:

Suggested change

Display the current value of the ``AWS_PROFILE`` or ``AWS_DEFAULT_PROFILE``

Display the current value of the :envvar:`AWS_PROFILE` or

:envvar:`AWS_DEFAULT_PROFILE`.

It won't display any differently, but it will index and search better.

Rycieos · 2021-01-29T16:38:40Z

docs/config.rst

+   :value: 1
+
+   Display the current value of the ``AWS_PROFILE`` or ``AWS_DEFAULT_PROFILE``
+   environment variables if they are set.


This would be the place to add link(s) to the tools that this is displaying.

I'm assuming this is for the AWS CLI, so maybe his link? https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-envvars.html

Suggested change

environment variables if they are set.

environment variables if they are set. These variables are used to

configure the `AWS CLI`_.

.. _`AWS CLI`: https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-envvars.html

I've added a slightly different link that lands on the AWS documentation about named profiles, this seems more targetted.

Rycieos · 2021-01-29T16:42:17Z

docs/functions/theme.rst

@@ -90,6 +90,12 @@ specific text and formatting may change.

   .. versionadded:: 2.0

+.. function:: _lp_aws_profile() -> var:lp_aws_profile
+
+   Returns :func:`_lp_analog_time` with color from :attr:`LP_COLOR_AWS_PROFILE`.


Suggested change

Returns :func:`_lp_analog_time` with color from :attr:`LP_COLOR_AWS_PROFILE`.

Returns :func:`_lp_aws_profile` with color from :attr:`LP_COLOR_AWS_PROFILE`.

Apologies for the careless cut-and-paste error - I should have spotted this!

Rycieos · 2021-01-29T16:44:49Z

liquidprompt

+    if [[ -n "${AWS_PROFILE-}" ]]; then
+        __lp_escape "${AWS_PROFILE##*/}"
+        lp_aws_profile=$ret
+    elif [[ -n "${AWS_DEFAULT_PROFILE-}" ]]; then
+        __lp_escape "${AWS_PROFILE##*/}"
+        lp_aws_profile=$ret


This whole block could be simplified a bit to reduce duplication:

Suggested change

if [[ -n "${AWS_PROFILE-}" ]]; then

__lp_escape "${AWS_PROFILE##*/}"

lp_aws_profile=$ret

elif [[ -n "${AWS_DEFAULT_PROFILE-}" ]]; then

__lp_escape "${AWS_PROFILE##*/}"

lp_aws_profile=$ret

local aws_profile="${AWS_PROFILE-${AWS_DEFAULT_PROFILE-}}"

if [[ -n $aws_profile ]]; then

__lp_escape "${aws_profile##*/}"

lp_aws_profile=$ret

Good call, done.

Rycieos · 2021-01-29T16:45:35Z

liquidprompt

+    local ret
+
+    if [[ -n "${AWS_PROFILE-}" ]]; then
+        __lp_escape "${AWS_PROFILE##*/}"


What is the reasoning behind removing everything before the final slash? Is it just copying what the Python env data function does? I'd like to see a comment explaining why.

Hah, I think it was probably copied from there, all those years ago. It serves no purpose here as it does for the virtualenv so I'll remove that.

Rycieos · 2021-01-29T16:46:25Z

liquidprompt

+_lp_aws_profile_color() {
+    _lp_aws_profile || return "$?"
+
+    lp_aws_profile_color="[${LP_COLOR_AWS_PROFILE}AWS:${lp_aws_profile}${NO_COL}]"


This AWS: string really should be turned into a "mark" configuration option, probably named LP_MARK_AWS_PROFILE.

You are right, it should but I have decided to remove this altogether; I added it as an afterthought and returning to it I think it is unnecessary and clutters the prompt.

If there was a desire for a marker, one could be added in a separate commit at a later stage.

Rycieos · 2021-01-29T18:46:14Z

liquidprompt

@@ -2730,7 +2758,7 @@ _lp_default_theme_prompt_template() {
        # add user, host and permissions colon
        PS1+="${LP_BRACKET_OPEN}${LP_USER}${LP_HOST}${LP_PERM}"

-        PS1+="${LP_PWD}${LP_DIRSTACK}${LP_BRACKET_CLOSE}${LP_SCLS}${LP_VENV}${LP_PROXY}"
+        PS1+="${LP_PWD}${LP_DIRSTACK}${LP_BRACKET_CLOSE}${LP_SCLS}${LP_AWS_PROFILE}${LP_VENV}${LP_PROXY}"


Here you have LP_AWS_PROFILE before LP_VENV, but in docs/theme/default.rst you have it after. The order needs to be the same, as specified in that documentation:

All of the available template sections are listed below. Their order is the default order if the user does not configure a different template.

I don't care which one comes first personally, just make them the same.

You also added it to the liquid.ps1 file. You don't need to do that, see the comment in that file:

This file is not updated with new template sections.

But if you do, make sure it is the same order as here.

I've removed this from liquid.ps1 and standardised the order elsewhere.

This will add `[profile_name]` to the prompt should it find either `AWS_PROFILE` or `AWS_DEFAULT_PROFILE` set in the environment. This is useful when switching between different AWS profiles and working with multiple accounts.

sagepe · 2021-02-26T11:10:20Z

First, apologies for the radio silence for the last month.

I think I've responded to all your feedback and noted bits in the relevant conversations above - I hope this it now in better shape!

I've rebased against master and squashed everything together into a single commit.

Rycieos

apologies for the radio silence for the last month.

No worries! This PR sat for 4 years, so another month is a drop in the bucket. Sorry it took someone so long to get to it.

Thanks for the fixups! This looks really good, I have no more issues with it.

Extension to liquidprompt#496 to add [AWS Vault](https://github.com/99designs/aws-vault) support. AWS Vault does not set the AWS_PROFILE/AWS_DEFAULT_PROFILE variables, but does add AWS_VAULT. Since this is philosophically the same as showing an AWS profile on the prompt, I think adding AWS_VAULT to the detection chain seems reasonable.

Extension to liquidprompt#496 to add [AWS Vault](https://github.com/99designs/aws-vault) support. AWS Vault does not set the AWS_PROFILE/AWS_DEFAULT_PROFILE variables, but does add AWS_VAULT. Since this is philosophically the same as showing an AWS profile on the prompt, I simply added AWS_VAULT to the detection chain.

Extension to #496 to add [AWS Vault](https://github.com/99designs/aws-vault) support. AWS Vault does not set the AWS_PROFILE/AWS_DEFAULT_PROFILE variables, but does add AWS_VAULT. Since this is philosophically the same as showing an AWS profile on the prompt, I simply added AWS_VAULT to the detection chain.

dolmen added dev env Data sources relating to development environments enhancement Feature request labels Dec 13, 2016

sagepe force-pushed the feature/aws-default-profile branch from f24d222 to 8521abb Compare January 29, 2021 11:43

sagepe changed the title ~~Add support for AWS_DEFAULT_PROFILE environment variable~~ Add support for AWS_PROFILE and AWS_DEFAULT_PROFILE environment variables Jan 29, 2021

Rycieos requested changes Jan 29, 2021

View reviewed changes

Rycieos reviewed Jan 29, 2021

View reviewed changes

Add support for AWS_PROFILE environment variable

0e552ba

This will add `[profile_name]` to the prompt should it find either `AWS_PROFILE` or `AWS_DEFAULT_PROFILE` set in the environment. This is useful when switching between different AWS profiles and working with multiple accounts.

sagepe force-pushed the feature/aws-default-profile branch from b0d2484 to 0e552ba Compare February 26, 2021 10:46

sagepe requested a review from Rycieos February 26, 2021 11:10

Rycieos approved these changes Feb 26, 2021

View reviewed changes

Rycieos added this to the v2.1 milestone Feb 26, 2021

Rycieos merged commit d5e8f5a into liquidprompt:master Feb 26, 2021

sagepe deleted the feature/aws-default-profile branch March 12, 2021 11:52

elerch mentioned this pull request Jul 6, 2021

Add AWS Vault support #679

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for AWS_PROFILE and AWS_DEFAULT_PROFILE environment variables #496

Add support for AWS_PROFILE and AWS_DEFAULT_PROFILE environment variables #496

sagepe commented Dec 6, 2016

dolmen commented Feb 10, 2017

sagepe commented Feb 14, 2017

Rycieos commented Jan 28, 2021

sagepe commented Jan 29, 2021

Rycieos left a comment

Rycieos Jan 29, 2021

sagepe Feb 26, 2021

Rycieos Jan 29, 2021

sagepe Feb 26, 2021

Rycieos Jan 29, 2021

sagepe Feb 26, 2021

Rycieos Jan 29, 2021

sagepe Feb 26, 2021

Rycieos Jan 29, 2021

sagepe Feb 26, 2021

Rycieos Jan 29, 2021

sagepe Feb 26, 2021

Rycieos Jan 29, 2021

sagepe Jan 29, 2021

Rycieos Jan 29, 2021

sagepe Feb 26, 2021

Rycieos Jan 29, 2021

sagepe Feb 26, 2021

sagepe commented Feb 26, 2021

Rycieos left a comment

		@@ -90,6 +90,12 @@ specific text and formatting may change.

		.. versionadded:: 2.0

		.. function:: _lp_aws_profile() -> var:lp_aws_profile

	.. function:: _lp_aws_profile() -> var:lp_aws_profile
	.. function:: _lp_aws_profile_color() -> var:lp_aws_profile_color

	__lp_escape "${AWS_PROFILE##*/}"
	__lp_escape "${AWS_DEFAULT_PROFILE##*/}"

	Display the current value of the ``AWS_PROFILE`` or ``AWS_DEFAULT_PROFILE``
	Display the current value of the :envvar:`AWS_PROFILE` or
	:envvar:`AWS_DEFAULT_PROFILE`.

-   environment variables if they are set.
+   environment variables if they are set. These variables are used to
+   configure the `AWS CLI`_.
+   .. _`AWS CLI`: https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-envvars.html

	Returns :func:`_lp_analog_time` with color from :attr:`LP_COLOR_AWS_PROFILE`.
	Returns :func:`_lp_aws_profile` with color from :attr:`LP_COLOR_AWS_PROFILE`.

Add support for AWS_PROFILE and AWS_DEFAULT_PROFILE environment variables #496

Add support for AWS_PROFILE and AWS_DEFAULT_PROFILE environment variables #496

Conversation

sagepe commented Dec 6, 2016

dolmen commented Feb 10, 2017

sagepe commented Feb 14, 2017

Rycieos commented Jan 28, 2021

sagepe commented Jan 29, 2021

Rycieos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagepe commented Feb 26, 2021

Rycieos left a comment

Choose a reason for hiding this comment