-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(core): use literal for stack.partition (under feature flag) #21420
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
8b5f370
to
95df59c
Compare
Pull request has been modified.
95df59c
to
5d3ab5e
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -867,6 +867,43 @@ describe('stack', () => { | |||
expect(stack.resolve(stack.region)).toEqual('es-norst-1'); | |||
}); | |||
|
|||
describe('@aws-cdk/core:enablePartitionLiterals', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this descriptive instead of listing the flag. Better readability that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done in the latest update; thanks
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. |
* AWS: | ||
* Fn::Join: | ||
* - "" | ||
* - - "arn:" | ||
* - Ref: AWS::Partition | ||
* - :iam::123456789876:root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are in Markdown. This needs triple backticks around it to be formatted in monospace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review, I'll get this fixed in the next push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
5d3ab5e
to
23bfcb3
Compare
Pull request has been modified.
23bfcb3
to
2b9eaf5
Compare
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.
2b9eaf5
to
56cc0df
Compare
The most recent push includes the results of The RegionInfo looks correct to me with 30 regions:
|
I'm confused about why the linter is failing. There are updates to READMEs in this change. |
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). |
1 similar comment
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
…#21420) 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. The feature flag to enable this behavior is `@aws-cdk/core:enablePartitionLiterals`. ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] 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*
closes #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.
The feature flag to enable this behavior is
@aws-cdk/core:enablePartitionLiterals
.All Submissions:
Adding new Unconventional Dependencies:
New Features
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