feature: Support multi region login for AWS ECR #857
Conversation
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
force-pushed
the
support-multi-region
branch
from
07451cc to
a98cecd
Compare
hhrygim
changed the title
|
@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']], |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙌
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_REGIONSthat can additionally be used to run the login against multiple regions.Main changes are in
aws.tswheregetRegionis replaced bygetRegionswhich will construct and return a list rather than a string. SincegetRegistriesDataalready returnsregDatasin a list due to its support for multi-aws-accounts, all I really needed to add was afor-loopwrapper to iterate on regions around the existing loop that iterates on account IDs.