fix(aws): enable when using .aws/credentials #4604
Conversation
offbyone
force-pushed
the
fix-aws-credentials-file
branch
3 times, most recently
from
8d7e72d
to
e1edd98
Compare
src/modules/aws.rs
Outdated
| config_section.contains_key("credential_process") | ||
| || config_section.contains_key("sso_start_url") | ||
| || credential_section.contains_key("credential_process") | ||
| || credential_section.contains_key("sso_start_url"), |
There was a problem hiding this comment.
has_credential_process_or_sso has been mostly concerned with the config file and has_defined_credentials with the credentials file. Given that, please move the has_defined_credentials into has_defined_credentials or change this function to first do the check on the config file and either return on success, or follow up with the credentials file check.
There was a problem hiding this comment.
That doesn't quite feel right to me. That checks for an actual active credential, which in this case would require me to invoke the credential process under the hood... not something we want to do in the prompt. This is just looking for the configuration that enables credentials, in the second place where it can be found. The AWS CLI allows for it to be configured in two places (which is a bad idea, for sure!) but right now Starship only looks in one of them.
If I'm going to change that, I think the name of those methods should change as well. I'll update the PR with the changes from the below, but I think this method's the right place for the code. If you insist otherwise, I'll move it, but it feels wrong to me.
There was a problem hiding this comment.
I would still prefer If you changed this code to do the checks sequentially, to only read each file based on necessity. But since it looks like they both will be read in most cases either way, keeping it this way would be fine too.
There was a problem hiding this comment.
That was my thinking too; the most common configuration will have both files read, so I don't think there's a lot of benefit or optimization in making that more complex. Doubly so because starship caches the read, so it's still pretty safe.
offbyone
force-pushed
the
fix-aws-credentials-file
branch
from
e1edd98
to
48aac57
Compare
fix(aws): enable when using .aws/credentials
Description
Check in the AWS credentials file for the
credential_processin order to enablethe
awsmodule, not just in the.aws/configfile (and all related configuredlocations for same.)
Motivation and Context
Closes #4603
Screenshots (if appropriate):
How Has This Been Tested?
Checklist: