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

Make aws-region optional #689

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ inputs:
required: false
aws-region:
description: 'AWS Region, e.g. us-east-2'
required: true
required: false
mask-aws-account-id:
description: >-
Whether to set the AWS account ID for these credentials as a secret value,
Expand All @@ -44,7 +44,7 @@ inputs:
assume an IAM role using a web identity. E.g., from within an Amazon EKS worker node
required: false
role-duration-seconds:
description: "Role duration in seconds (default: 6 hours, 1 hour for OIDC/specified aws-session-token)"
description: 'Role duration in seconds (default: 6 hours, 1 hour for OIDC/specified aws-session-token)'
required: false
role-session-name:
description: 'Role session name (default: GitHubActions)'
Expand Down
2 changes: 1 addition & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ async function run() {
const accessKeyId = core.getInput('aws-access-key-id', { required: false });
const audience = core.getInput('audience', { required: false });
const secretAccessKey = core.getInput('aws-secret-access-key', { required: false });
const region = core.getInput('aws-region', { required: true });
const region = core.getInput('aws-region', { required: true }) || process.env.AWS_REGION || process.env.AWS_DEFAULT_REGION;
Copy link
Contributor

Choose a reason for hiding this comment

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

Set required: false 🙂

const sessionToken = core.getInput('aws-session-token', { required: false });
const maskAccountId = core.getInput('mask-aws-account-id', { required: false });
const roleToAssume = core.getInput('role-to-assume', {required: false});
Expand Down
17 changes: 17 additions & 0 deletions index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,23 @@ describe('Configure AWS Credentials', () => {
expect(core.setFailed).toHaveBeenCalledWith('Region is not valid: $AWS_REGION');
});

test('aws-region should be optional and use AWS_REGION instead', async () => {
const mockInputs = { ...CREDS_INPUTS }
core.getInput = jest.fn().mockImplementation(mockGetInput(mockInputs))
process.env.AWS_REGION = "fake-region"
await run()
expect(core.exportVariable).toHaveBeenCalledWith("AWS_REGION", "fake-region")
expect(core.exportVariable).toHaveBeenCalledWith("AWS_DEFAULT_REGION", "fake-region")
})
test('aws-region should be optional and use AWS_DEFAULT_REGION if AWS_REGION is not available', async () => {
const mockInputs = { ...CREDS_INPUTS }
core.getInput = jest.fn().mockImplementation(mockGetInput(mockInputs))
process.env.AWS_DEFAULT_REGION = "fake-default-region"
await run()
expect(core.exportVariable).toHaveBeenCalledWith("AWS_REGION", "fake-default-region")
expect(core.exportVariable).toHaveBeenCalledWith("AWS_DEFAULT_REGION", "fake-default-region")
})

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like two more tests here:

One to make sure that when the action input, and both AWS_REGION and AWS_DEFAULT_REGION are all set, the action will prefer the action input value over all others.

Another test to verify that when both AWS_REGION and AWS_DEFAULT_REGION are set, it prefers AWS_REGION

test('throws error if access key id exists but missing secret access key', async () => {
process.env.SHOW_STACK_TRACE = 'false';
const inputsWIthoutSecretKey = {...ASSUME_ROLE_INPUTS}
Expand Down