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.

Sign up for GitHub

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

Jump to bottom

Make region optional #431

Closed
wants to merge 2 commits into from
Closed

Make region optional #431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b6ee73
Make region optional
nitrocode Apr 28, 2022
18d55d7
Make region optional
nitrocode Apr 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,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 Down
3 changes: 2 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const MAX_TAG_VALUE_LENGTH = 256;
const SANITIZATION_CHARACTER = '_';
const ROLE_SESSION_NAME = 'GitHubActions';
const REGION_REGEX = /^[a-z0-9-]+$/g;
const AWS_CURRENT_REGION = process.env.AWS_REGION
Copy link
Author

Choose a reason for hiding this comment

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

or should this be AWS_DEFAULT_REGION ?

Copy link

Choose a reason for hiding this comment

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

I don't think so; you want to be able to override the default region, which is what AWS_REGION is for

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't set this as a global variable - instead use process.env where the value is needed.


async function assumeRole(params) {
// Assume a role to get short-lived credentials using longer-lived credentials.
Expand Down Expand Up @@ -264,7 +265,7 @@ async function run() {
// Get inputs
const accessKeyId = core.getInput('aws-access-key-id', { 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: false }) || AWS_CURRENT_REGION;
Copy link
Contributor

Choose a reason for hiding this comment

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

Like I mentioned above, we should use process.env.XXX instead of a global variable (after all you're only using it on this line)

To add on to the discussion above regarding AWS_REGION vs AWS_DEFAULT_REGION... why not both? Action input > AWS_REGION > AWS_DEFAULT_REGION should work here no?

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