fix(appconfig-alpha): extensions always create cdk diff #28264
Conversation
github-actions
bot
added
bug
aws-cdk-automation
added
the
pr/needs-community-review
aws-cdk-automation
removed
the
pr/needs-community-review
| ...(options?.parameters ? { parameters: options.parameters } : {}), | ||
| }); | ||
| this.addExtensionAssociation(extension, options); | ||
| } | ||
|
|
||
| private addExtensionAssociation(extension: IExtension, options?: ExtensionOptions) { | ||
| new CfnExtensionAssociation(this.scope, `AssociationResource${this.getExtensionAssociationHash(extension)}`, { | ||
| const versionNumber = options?.latestVersionNumber ? options?.latestVersionNumber + 1 : 1; | ||
| const name = extension.name ?? this.getExtensionDefaultName(); |
There was a problem hiding this comment.
| const name = extension.name ?? this.getExtensionDefaultName(); | |
| const name = extension.name ?? options?.name ?? this.getExtensionDefaultName(); |
name can be passed via options as well.
I think that getExtensionDefaultName should be used here as well for consistency.
To avoid this default to chaining after initialization we could think about making this and this required and use getExtensionDefaultName for imported extensions.
This may be a better approach, not sure how much refactoring it would require.
There was a problem hiding this comment.
We actually don't need the options parameter here, I just got rid of it.
For the second point, the scope in these two places are different. In the link you attached, the user would pass in the logical id which might be something like MyExtension so here adding the -Extension there would be redundant. Here the scope is the parent so adding the -Extension prevents it from being named the same as the parent. For requiring the name as a parameter, this goes against the CDK guidelines as we want to try to take as much of the work off the user. This was additionally a customer pain point when working with the L1 constructs.
There was a problem hiding this comment.
Ok clear, thanks for the follow-up and the parameter cleanup 👍
aws-cdk-automation
added
the
pr/needs-maintainer-review
vinayak-kukreja
changed the title
There was a problem hiding this comment.
Hey @chenjane-dev thank you for contributing to the cdk. And, @lpizzinidev thank you for reviewing the pr.
|
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). |
|
@Mergifyio update |
❌ Mergify doesn't have permission to updateFor security reasons, Mergify can't update this pull request. Try updating locally. |
mergify
bot
dismissed
vinayak-kukreja’s stale review
Pull request has been modified.
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). |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
Fixing extensions CDK diff since before it would always generate a new diff even if nothing changed.
Closes #27676.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license