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

fix: update AWS_S3_USE_ARN_REGION to take precedence over value set i… #3513

Open
wants to merge 3 commits into
base: v4-development
Choose a base branch
from

Conversation

peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Oct 15, 2024

…n config file

Description

This updates AmazonS3Config use-arn-region to take the value set in environment variable over that set in the ~/.aws/config file. This follows other env var + config file precedence.

Motivation and Context

dotnet-7073

Testing

dry run in v4 passes
Added unit tests

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license


private bool forcePathStyle = false;
private bool useAccelerateEndpoint = false;
private S3UsEast1RegionalEndpointValue? s3UsEast1RegionalEndpointValue;
private readonly string legacyUSEast1GlobalRegionSystemName = RegionEndpoint.USEast1.SystemName;

private static CredentialProfileStoreChain credentialProfileChain = new CredentialProfileStoreChain();
private static CredentialProfileStoreChain credentialProfileChain = new CredentialProfileStoreChain(Environment.GetEnvironmentVariable(AwsConfigFileEnvName));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to leaving this as is. We added support for AWS_CONFIG_FILE a couple years ago and when this class was created, this environment variable wasn't available. Now that it is, it makes sense for the CredentialProfileStoreChain to check this value to see if it should look somewhere else for the config file.

Given that the constructor accepts a ProfileLocation, I thought it made sense to add it here. If it isn't set, then it will just continue to work as it did previously.

Copy link
Contributor

@dscpinheiro dscpinheiro Oct 17, 2024

Choose a reason for hiding this comment

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

I prefer changing this in a separate PR, since there are other configs that need to fixed:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, given there are other services that need the update, we can do it all in a separate pr👍


private bool forcePathStyle = false;
private bool useAccelerateEndpoint = false;
private S3UsEast1RegionalEndpointValue? s3UsEast1RegionalEndpointValue;
private readonly string legacyUSEast1GlobalRegionSystemName = RegionEndpoint.USEast1.SystemName;

private static CredentialProfileStoreChain credentialProfileChain = new CredentialProfileStoreChain();
private static CredentialProfileStoreChain credentialProfileChain = new CredentialProfileStoreChain(Environment.GetEnvironmentVariable(AwsConfigFileEnvName));
Copy link
Contributor

@dscpinheiro dscpinheiro Oct 17, 2024

Choose a reason for hiding this comment

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

I prefer changing this in a separate PR, since there are other configs that need to fixed:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants