fix(cloudfront): fromOriginAccessIdentityName is a misnomer#20772
Conversation
|
|
|
Fixes #20141 |
9506d28 to
5948f8c
Compare
|
I had to take two shortcuts:
For both, I'm happy about suggestions, but if CI is green, then from my point of view, the PR can already be merged as-is. |
peterwoodworth
left a comment
There was a problem hiding this comment.
Like you said, It would be nice to verify that the deprecated prop works. We have a testDeprecated which behaves similarly to test. Check out an example of this here https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-elasticsearch/test/domain.test.ts
Pull request has been modified.
|
I've added a test for the now deprecated |
eaf082c to
add8357
Compare
|
I think the tests you have here are plenty sufficient @dbartholomae, I'll give this an approval once you make your desired rebase |
add8357 to
d9a9f71
Compare
|
@peterwoodworth here you are :) |
peterwoodworth
left a comment
There was a problem hiding this comment.
LGTM! going to let another team member take a quick peek to double check that deprecation was handled properly here, then it should be merged
kaizencc
left a comment
There was a problem hiding this comment.
The deprecation here looks fine @peterwoodworth, @dbartholomae. One call out for something weird in the Cloudformation docs -- not sure why a return value exists in the docs and is also "not supported".
| * @attribute | ||
| * @deprecated use originAccessIdentityId instead | ||
| */ | ||
| public get originAccessIdentityName() { |
There was a problem hiding this comment.
I do want to call out that Id as an @attribute return value is allegedly "not currently supported by CloudFormation".
If true, and Cloudformation does not actually return this value, I think we should @deprecate public get originAccessIdentiyName and not supply an alternative. It all depends on whether cloudformation does return the Id, which would require someone to test this out themselves and confirm.
There was a problem hiding this comment.
Or if they would support it in the future?
There was a problem hiding this comment.
I tested it and it's currently supported if I create an OAI ID and reference the attribute in a CfnOutput. Seems to just work
There was a problem hiding this comment.
going to remove the label and let this merge
There was a problem hiding this comment.
correction - we just use ref here which does return the OAI Id. But, using the ID attribute still works will return the exact same value as ref so...
|
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). |
fixes aws#20141 ---- ### 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 * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*
fixes #20141
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license