Skip to content
This repository has been archived by the owner on Aug 16, 2022. It is now read-only.

feat: Support AWS partitions #842

Merged
merged 3 commits into from
May 9, 2022
Merged

feat: Support AWS partitions #842

merged 3 commits into from
May 9, 2022

Conversation

disq
Copy link
Member

@disq disq commented May 4, 2022

Should fix #806 (untested)

@disq disq requested a review from a team as a code owner May 4, 2022 10:02
@disq disq requested review from irmatov, bbernays and roneli and removed request for a team May 4, 2022 10:02
@github-actions github-actions bot added the feat label May 4, 2022
Copy link
Contributor

@bbernays bbernays left a comment

Choose a reason for hiding this comment

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

LGTM

@bbernays
Copy link
Contributor

bbernays commented May 4, 2022

Are there any tests we want to add for this?

@disq
Copy link
Member Author

disq commented May 5, 2022

Are there any tests we want to add for this?

added one, but not feeling very creative. Would need e2e to make sure I suppose

Copy link
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

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

Ill comment as I am not 100% sure, but this means we run on all partitions now?
Should we add a config option to define what partitions we want to execute on? i.e by default don't have fetch from gov partitions, as I assume, most users don't have those regions enabled.
Other then that LGTM.

@disq
Copy link
Member Author

disq commented May 6, 2022

Ill comment as I am not 100% sure, but this means we run on all partitions now? Should we add a config option to define what partitions we want to execute on? i.e by default don't have fetch from gov partitions, as I assume, most users don't have those regions enabled. Other then that LGTM.

We run on what describeregions return, or regions that are explicitly specified in the hcl. @roneli
Shouldn't matter for normal users as describeregions only return aws-partition-regions for them.

Copy link
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

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

LGTM

@bbernays
Copy link
Contributor

bbernays commented May 8, 2022

Ill comment as I am not 100% sure, but this means we run on all partitions now? Should we add a config option to define what partitions we want to execute on? i.e by default don't have fetch from gov partitions, as I assume, most users don't have those regions enabled. Other then that LGTM.

We run on what describeregions return, or regions that are explicitly specified in the hcl. @roneli Shouldn't matter for normal users as describeregions only return aws-partition-regions for them.

To add on to what @disq said: AWS credentials are scoped to a specific partition. So if a user uses a single provider block with multiple accounts in it they could still specify specific regions on a per account basis

@disq disq merged commit 6976653 into main May 9, 2022
@disq disq deleted the feat/support-aws-partitions branch May 9, 2022 08:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support China Region
3 participants