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

Read region value for non default profiles #880

Merged
merged 1 commit into from
Nov 12, 2024
Merged

Conversation

gcbeattyAWS
Copy link
Contributor

@gcbeattyAWS gcbeattyAWS commented Oct 30, 2024

Issue #, if available: N/A

Description of changes:

Why is this change being made?

The problem we are trying to solve is that the dotnet tool does not take into account the AWS region when a non default profile is being used. It will use the region set on the default profile (if available) instead.

Also, this change also updates the cdk bootstrap version

For example consider the following profile setups.
Scenario 1

[test]
access-key=xyz
secret-key=abc
region=us-west-2

[default]
access-key=xyz
secret-key=abc
region=us-east-1

Running the program with

AWS.Deploy.CLI.exe deploy --profile test

will result in region us-east-1 being used rather than us-west-2

As a workaround, we let the user know to provide the --region parameter when a non default profile is being used

AWS.Deploy.CLI.exe deploy --profile test --region us-west-2

Scenario 2: There is also another scenario that can occur with the following profiles setup (no default profile/default profile doesn't have a region)

[test]
access-key=xyz
secret-key=abc
region=us-west-2

When running with
AWS.Deploy.CLI.exe deploy --profile test

this will prompt the user to enter a region as input

Select AWS Region
-----------------
1 : af-south-1 (Africa (Cape Town))
2 : ap-east-1 (Asia Pacific (Hong Kong))
Choose option:

Both of these scenarios are not ideal because we are requiring the user to manually select the region each time (either by providing --region flag as input or by doing as input in command line)

What is this change being made?

  1. This change updates ResolveAWSCredentials function to read the profile's region and return it from the function. This allows the next function (ResolveAWSRegion to use the returned profile's region as input.
  2. This change also refactors some of AWSUtilities so that some of the credential chain logic is injected so that unit tests can be written for this class.

How is this change tested?

  1. I tested the two scenarios described above.

Scenario 1

[test]
access-key=xyz
secret-key=abc
region=us-west-2

[default]
access-key=xyz
secret-key=abc
region=us-east-1

Running the program with

AWS.Deploy.CLI.exe deploy --profile test

I validated that the program auto detected us-west-2 rather than us-east-

Scenario 2: There is also another scenario that can occur with the following profiles setup (no default profile/default profile doesn't have a region)

[test]
access-key=xyz
secret-key=abc
region=us-west-2

When running with
AWS.Deploy.CLI.exe deploy --profile test

I validated that i am not prompted anymore for a region, and us-west-2 is auto detected.

I also validated that the existing functionality of explicitly specifying a region still works. For example, even with this profile

[test]
access-key=xyz
secret-key=abc
region=us-east-2

i ran AWS.Deploy.CLI.exe deploy --profile test --region us-west-1 and validated us-west-1 was chosen.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@gcbeattyAWS gcbeattyAWS reopened this Oct 30, 2024
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 62.78%. Comparing base (589d9b9) to head (ed86700).
Report is 2 commits behind head on dev.

Files with missing lines Patch % Lines
src/AWS.Deploy.CLI/AWSUtilities.cs 88.88% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #880      +/-   ##
==========================================
+ Coverage   62.39%   62.78%   +0.38%     
==========================================
  Files         279      282       +3     
  Lines       10908    10922      +14     
  Branches     1515     1517       +2     
==========================================
+ Hits         6806     6857      +51     
+ Misses       3565     3526      -39     
- Partials      537      539       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gcbeattyAWS gcbeattyAWS force-pushed the gcbeatty/deployregion branch 3 times, most recently from 2565a7a to f935b14 Compare October 31, 2024 20:56
@gcbeattyAWS gcbeattyAWS marked this pull request as ready for review October 31, 2024 22:55
src/AWS.Deploy.CLI/AWSUtilities.cs Outdated Show resolved Hide resolved
src/AWS.Deploy.CLI/AWSUtilities.cs Show resolved Hide resolved
src/AWS.Deploy.CLI/AWSUtilities.cs Outdated Show resolved Hide resolved
src/AWS.Deploy.CLI/Commands/CommandFactory.cs Outdated Show resolved Hide resolved
src/AWS.Deploy.CLI/CredentialProfileStoreChainFactory.cs Outdated Show resolved Hide resolved
src/AWS.Deploy.CLI/IFallbackCredentialsFactoryFactory.cs Outdated Show resolved Hide resolved
src/AWS.Deploy.CLI/ISharedCredentialsFileFactory.cs Outdated Show resolved Hide resolved
src/AWS.Deploy.CLI/ISharedCredentialsFileFactory.cs Outdated Show resolved Hide resolved
src/AWS.Deploy.CLI/SharedCredentialsFileFactory.cs Outdated Show resolved Hide resolved
src/AWS.Deploy.CLI/SharedCredentialsFileFactory.cs Outdated Show resolved Hide resolved
@gcbeattyAWS gcbeattyAWS force-pushed the gcbeatty/deployregion branch from 6d8583e to ae858c0 Compare November 6, 2024 16:46
src/AWS.Deploy.CLI/AWSUtilities.cs Outdated Show resolved Hide resolved
src/AWS.Deploy.Orchestration/CDK/CDKBootstrapTemplate.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@philasmar philasmar left a comment

Choose a reason for hiding this comment

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

The PR should target dev not main. Also, you should add a change file to the PR https://github.com/awslabs/aws-dotnet-messaging/blob/main/CONTRIBUTING.md#adding-a-change-file-to-your-contribution-branch.

@gcbeattyAWS gcbeattyAWS changed the base branch from main to dev November 8, 2024 21:12
@gcbeattyAWS gcbeattyAWS force-pushed the gcbeatty/deployregion branch from 1154162 to 80a0d73 Compare November 11, 2024 14:00
@gcbeattyAWS gcbeattyAWS force-pushed the gcbeatty/deployregion branch from 80a0d73 to ed86700 Compare November 12, 2024 19:53
@gcbeattyAWS gcbeattyAWS merged commit e810ffe into dev Nov 12, 2024
11 checks passed
@gcbeattyAWS gcbeattyAWS deleted the gcbeatty/deployregion branch November 12, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants