feat(core): use literal for stack.partition (under feature flag) by jgrebholz · Pull Request #21420 · aws/aws-cdk

jgrebholz · 2022-08-02T13:51:01Z

When a Stack's region is a string literal, we can return a string
literal from Stack.partition, resulting in easier to read generated
templates.

The feature flag to enable this behavior is @aws-cdk/core:enablePartitionLiterals.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-08-02T13:51:11Z

TheRealAmazonKendra

For any customers relying on the output of partition(), this will be a breaking change, as evidenced by the integration tests being broken in other parts of the code base.

I suppose that it can be submitted under a feature flag but I'm not sure the value of it is worth adding another one.

Pull request has been modified.

jgrebholz · 2022-08-02T21:51:24Z

Thanks for your review @TheRealAmazonKendra I've updated the PR to include a feature flag, a minor update to the README, and confirmed the previously failing aws-iam integ tests are passing now.

TheRealAmazonKendra

Looks like there are more test failures to attend to. Note that integration tests have feature flags enabled by default so anywhere that's using this function without a token will update. This may take a few rounds to discover all the locations of the failures.

TheRealAmazonKendra · 2022-08-02T23:29:15Z

packages/@aws-cdk/core/test/stack.test.ts

    expect(stack.resolve(stack.region)).toEqual('es-norst-1');
  });

+  describe('@aws-cdk/core:enablePartitionLiterals', () => {


Please make this descriptive instead of listing the flag. Better readability that way.

This is done in the latest update; thanks

TheRealAmazonKendra · 2022-08-02T23:36:16Z

Oh, I almost forgot, now that this is behind a feature flag, please update your PR to reflect that. We have specific formatting instructions in our contributing guide.

rix0rrr · 2022-08-08T07:28:37Z

packages/@aws-cdk/cx-api/lib/features.ts

+ *   AWS:
+ *     Fn::Join:
+ *       - ""
+ *       - - "arn:"
+ *         - Ref: AWS::Partition
+ *         - :iam::123456789876:root


Comments are in Markdown. This needs triple backticks around it to be formatted in monospace.

Thanks for your review, I'll get this fixed in the next push

rix0rrr

This PR changes suspiciously little. I would have expected literally every integration test to blow up, and how confident are we that RegionInfo is correct for all these regions?

Pull request has been modified.

closes aws#4092 When a Stack's region is a string literal, we can return a string literal from Stack.partition, resulting in easier to read generated templates.

jgrebholz · 2022-08-11T06:59:38Z

The most recent push includes the results of yarn integ --update-on-failed for each of the failing integration tests needed for my local run of ./build.sh to succeed. I'll review the results of the build fleet tomorrow and update as needed. I'm not sure why so many integ tests reported UNCHANGED on their snapshot comparisons, but I'm happy to investigate any that you think should be objecting. Is it possible they're using AWS.Region or a stack parameter for region?

The RegionInfo looks correct to me with 30 regions:

$ cat built-ins.generated.ts | grep FactName.PARTITION | awk '{print $3, $7;}'
"af-south-1", "aws"
"ap-east-1", "aws"
"ap-northeast-1", "aws"
"ap-northeast-2", "aws"
"ap-northeast-3", "aws"
"ap-south-1", "aws"
"ap-southeast-1", "aws"
"ap-southeast-2", "aws"
"ap-southeast-3", "aws"
"ca-central-1", "aws"
"cn-north-1", "aws-cn"
"cn-northwest-1", "aws-cn"
"eu-central-1", "aws"
"eu-north-1", "aws"
"eu-south-1", "aws"
"eu-south-2", "aws"
"eu-west-1", "aws"
"eu-west-2", "aws"
"eu-west-3", "aws"
"me-south-1", "aws"
"sa-east-1", "aws"
"us-east-1", "aws"
"us-east-2", "aws"
"us-gov-east-1", "aws-us-gov"
"us-gov-west-1", "aws-us-gov"
"us-iso-east-1", "aws-iso"
"us-iso-west-1", "aws-iso"
"us-isob-east-1", "aws-iso-b"
"us-west-1", "aws"
"us-west-2", "aws"

TheRealAmazonKendra · 2022-08-11T15:54:39Z

I'm confused about why the linter is failing. There are updates to READMEs in this change.

mergify · 2022-08-12T14:32:11Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2022-08-12T14:35:12Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-08-12T15:35:41Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 0f82798
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-08-12T15:35:54Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Aug 2, 2022

aws-cdk-automation requested a review from a team August 2, 2022 13:51

TheRealAmazonKendra previously requested changes Aug 2, 2022

View reviewed changes

jgrebholz force-pushed the feat/partition_literal branch from 8b5f370 to 95df59c Compare August 2, 2022 21:40

jgrebholz force-pushed the feat/partition_literal branch from 95df59c to 5d3ab5e Compare August 2, 2022 21:46

TheRealAmazonKendra previously requested changes Aug 2, 2022

View reviewed changes

TheRealAmazonKendra changed the title ~~feat(core): use literal for stack.partition (#4092)~~ feat(core): use literal for stack.partition Aug 2, 2022

rix0rrr reviewed Aug 8, 2022

View reviewed changes

jgrebholz force-pushed the feat/partition_literal branch from 5d3ab5e to 23bfcb3 Compare August 11, 2022 06:28

jgrebholz changed the title ~~feat(core): use literal for stack.partition~~ feat(core): use literal for stack.partition (under feature flag) Aug 11, 2022

jgrebholz force-pushed the feat/partition_literal branch from 23bfcb3 to 2b9eaf5 Compare August 11, 2022 06:33

jgrebholz added 4 commits August 11, 2022 06:43

feat(core): Use literal for stack.partition (aws#4092)

fff758d

closes aws#4092 When a Stack's region is a string literal, we can return a string literal from Stack.partition, resulting in easier to read generated templates.

fix(aws-eks): integration test --update-on-failed

0cd2981

fix(codepipeline): integration tests --update-on-failed

8c48c5b

fix(dynamodb): integration tests --update-on-failed

56cc0df

jgrebholz force-pushed the feat/partition_literal branch from 2b9eaf5 to 56cc0df Compare August 11, 2022 06:43

TheRealAmazonKendra added pr-linter/exempt-readme The PR linter will not require README changes and removed pr-linter/exempt-readme The PR linter will not require README changes labels Aug 11, 2022

TheRealAmazonKendra added the pr-linter/exempt-readme The PR linter will not require README changes label Aug 11, 2022

rix0rrr approved these changes Aug 12, 2022

View reviewed changes

Merge branch 'main' into feat/partition_literal

0f82798

mergify bot merged commit 401b428 into aws:main Aug 12, 2022

Conversation

jgrebholz commented Aug 2, 2022 • edited by rix0rrr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Aug 2, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

jgrebholz commented Aug 2, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

jgrebholz Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra commented Aug 2, 2022

Uh oh!

rix0rrr Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrebholz Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

rix0rrr left a comment

Choose a reason for hiding this comment

Uh oh!

jgrebholz commented Aug 11, 2022

Uh oh!

TheRealAmazonKendra commented Aug 11, 2022

Uh oh!

mergify bot commented Aug 12, 2022

Uh oh!

mergify bot commented Aug 12, 2022

Uh oh!

aws-cdk-automation commented Aug 12, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jgrebholz commented Aug 2, 2022 •

edited by rix0rrr

Loading

rix0rrr Aug 8, 2022 •

edited

Loading