Skip to content

feature: Support multi region login for AWS ECR #857

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

Closed
wants to merge 1 commit into from

Conversation

hhrygim
Copy link

@hhrygim hhrygim commented Feb 28, 2025

This adds support for allowing AWS ECR logins via multiple regions.

Functionality is quite similar to the existing support for multi-AWS accounts. This adds in a new valid environment variable AWS_REGIONS that can additionally be used to run the login against multiple regions.

Main changes are in aws.ts where getRegion is replaced by getRegions which will construct and return a list rather than a string. Since getRegistriesData already returns regDatas in a list due to its support for multi-aws-accounts, all I really needed to add was a for-loop wrapper to iterate on regions around the existing loop that iterates on account IDs.

This adds support for allowing AWS ECR logins via multiple regions.

Functionality is quite similar to the existing support for multi-AWS
accounts. This adds in a new valid environment variable `AWS_REGIONS`
that can additionally be used to run the login against multiple regions.

Main changes are in `aws.ts` where `getRegion` is replaced by
`getRegions` which will construct and return a list rather than a
string. Since `getRegistriesData` already returns `regDatas` in a list
due to its support for multi-aws-accounts, all I really needed to add
was a `for-loop` wrapper to iterate on regions around the existing loop
that iterates on account IDs.

Signed-off-by: Helen Lim <hlim2@atlassian.com>
@hhrygim hhrygim force-pushed the support-multi-region branch from 07451cc to a98cecd Compare February 28, 2025 16:42
@hhrygim hhrygim changed the title feature: Support multi reagion login for AWS ECR feature: Support multi region login for AWS ECR Feb 28, 2025
@hhrygim
Copy link
Author

hhrygim commented Mar 3, 2025

@crazy-max Wondering if you have any thoughts on this PR and if we think this is a useful feature to add. Not sure if I should open up an issue as well - lmk

['876820548815.dkr.ecr.cn-north-1.amazonaws.com.cn', undefined, ['cn-north-1']],
['390948362332.dkr.ecr.cn-northwest-1.amazonaws.com.cn', undefined, ['cn-northwest-1']],
['public.ecr.aws', undefined, ['us-east-1']],
['012345678901.dkr.ecr.eu-west-3.amazonaws.com', 'us-west-1,us-east-1', ['eu-west-3', 'us-west-1', 'us-east-1']],
Copy link
Member

@crazy-max crazy-max Mar 14, 2025

Choose a reason for hiding this comment

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

How would this work if registry is scoped to eu-west-3 but region is set to us-west-1? That looks error prone to me if registry is not meaningful anymore. cc @Flydiverny

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm following this and the previously existing AWS_ACCOUNT_IDS (unless I misunderstood the purpose of both features) are more work arounds to not supporting #87 ? 🤔
Maybe it would be keep code simpler to implement support for multiple registries input and make the usage more clear.

Copy link
Member

Choose a reason for hiding this comment

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

unless I misunderstood the purpose of both features) are more work arounds to not supporting #87 ? 🤔

Yes this looks slightly related to multi registries support. Maybe better to consider that instead of custom implementation for just aws.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks guys, I agree that support for multiple registries is a better use-case here. Ill take a look at re-opening a new pr when I get a second 🙌

@crazy-max crazy-max self-assigned this Mar 28, 2025
@crazy-max crazy-max closed this Apr 22, 2025
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