refactor(diff): make dedicated file and class for incorporating changeset to templateDiff#30332
refactor(diff): make dedicated file and class for incorporating changeset to templateDiff#30332mergify[bot] merged 58 commits intomainfrom
Conversation
…s from the template with what's in the changeset
| changeSetResources: types.ChangeSetResources; | ||
|
|
||
| constructor( | ||
| args: { |
There was a problem hiding this comment.
Nit: The convention we typically go for is constructor arguments being passed in via a defined interface. Going a little deeper, we try to consolidate all required property on a props interface and all optional properties on an options interface. Typically the props interface would extend the options interface. Something like:
interface TemplateAndChangeSetDiffMergerOptions {
/*
* Description of property
*
* @default - description of default
*/
readonly changeSetResources?: types.ChangeSetResources;
}export interface TemplateAndChangeSetDiffMergerProps extends TemplateAndChangeSetDiffMergerOptions {
/*
* Description of property
*/
readonly changeSet: DescribeChangeSetOutput;
}Then,
constructor(props: TemplateAndChangeSetDiffMergerProps) {}
packages/@aws-cdk/cloudformation-diff/lib/diff/template-and-changeset-diff-merger.ts
Outdated
Show resolved
Hide resolved
| changeSet: DescribeChangeSetOutput | undefined; | ||
| changeSetResources: types.ChangeSetResources; |
There was a problem hiding this comment.
Can we give these a public or private?
| } | ||
| change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => { | ||
| if (type === 'Property') { | ||
| if (!this.changeSetResources[logicalId]) { |
There was a problem hiding this comment.
Is this essentially just a new resource being added?
There was a problem hiding this comment.
This is us writing over the "ChangeImpact" that was computed from the template difference, and instead using the ChangeImpact that is included from the ChangeSet. Using the ChangeSet ChangeImpact is more accurate. The ChangeImpact tells us what the consequence is of changing the field. If changing the field causes resource replacement (e.g., changing the name of an IAM role requires deleting and replacing the role), then ChangeImpact is "Always".
There was a problem hiding this comment.
I am going to include this as a comment in the code
| // otherwise, defer to the changeImpact from the template diff | ||
| } | ||
| } else if (type === 'Other') { | ||
| switch (name) { |
There was a problem hiding this comment.
Are there any cases in here that wouldn't be Metadata, like an undefined or something?
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE; | ||
| (value as types.PropertyDifference<any>).isDifferent = false; | ||
| break; | ||
| // otherwise, defer to the changeImpact from the template diff |
There was a problem hiding this comment.
What case would be an otherwise situation? Would it make sense to make undefined the default case?
There was a problem hiding this comment.
I just want to refactor in this code change, so this is just a copy of what already exists being lifted into this new file. Not sure what would be an otherwise situation, though
There was a problem hiding this comment.
Got it. It makes sense to leave it as is for now.
|
|
||
| public findResourceImports(): string[] { | ||
| const importedResourceLogicalIds = []; | ||
| for (const resourceChange of this.changeSet?.Changes ?? []) { |
There was a problem hiding this comment.
Why would changeSet be undefined? It looks like it is a required property used to construct this class.
There was a problem hiding this comment.
This is just me being extra defensive... we could get rid of that, but I'd rather just be overly cautious?
There was a problem hiding this comment.
I don't think its needed though since you can't construct the class without a changeSet provided. Because of that we know changeSet can never be undefined.
| const importedResourceLogicalIds = []; | ||
| for (const resourceChange of this.changeSet?.Changes ?? []) { | ||
| if (resourceChange.ResourceChange?.Action === 'Import') { | ||
| importedResourceLogicalIds.push(resourceChange.ResourceChange.LogicalResourceId!); |
There was a problem hiding this comment.
Any chance we can avoid using !? Maybe just a check and throw an error if the resource ID is undefined? This way I think the compiler will know it wouldn't be undefined. Something like this:
const logicalResourceId = resourceChange.ResourceChange.LogicalResourceId;
if (logicalResourceId === undefined) {
throw new Error('...');
}
importedResourceLogicalIds.push(logicalResourceId);There was a problem hiding this comment.
great catch! I didn't change this from the refactor. I would like to change by making the function signature (string | undefined)[]
colifran
left a comment
colifran
left a comment
There was a problem hiding this comment.
Love the refactor! This looks great so far. I just have a few minor stylistic suggestions and questions.
aws-cdk-automation
removed
the
pr/needs-maintainer-review
|
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). |
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
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). |
…eset to templateDiff (aws#30332) ### Reason for this change I am making this change as part of aws#30268, but implementing the bug fix in a satisfactory way is becoming much, much, much more difficult than I thought it would. As it's now possible to view the changed values before and after a changeset is applied by using the DescribeChangeSets api with IncludePropertyValues, but the API is difficult to use because of not being supported in all regions, not including StatusReason, and being unable to paginate. So, I want to make that fix in a separate PR, once this refactor change is done. ### Description of changes * A ton of unit tests and moved changeset diff logic into a dedicated class and file. ### Description of how you validated changes * Many unit tests, integration tests, and manual tests ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
|
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
3 participants
Reason for this change
I am making this change as part of #30268, but implementing the bug fix in a satisfactory way is becoming much, much, much more difficult than I thought it would. As it's now possible to view the changed values before and after a changeset is applied by using the DescribeChangeSets api with IncludePropertyValues, but the API is difficult to use because of not being supported in all regions, not including StatusReason, and being unable to paginate. So, I want to make that fix in a separate PR, once this refactor change is done.
Description of changes
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license