integ-runner: What should be the message: "If these destructive changes are necessary, please indicate this on the PR"

[caevv]

Describe the issue

What should the PR contain to pass through this error?

!!! This test contains destructive changes !!!
    Stack: aws-cdk-lambda-1 - Resource: MyLambdaServiceRole4539ECB6 - Impact: WILL_DESTROY
    Stack: aws-cdk-lambda-1 - Resource: AliasAliasPermissionAF30F9E8 - Impact: WILL_REPLACE
    Stack: aws-cdk-lambda-1 - Resource: AliasFunctionUrlDC6EC566 - Impact: WILL_REPLACE
    Stack: aws-cdk-lambda-1 - Resource: Aliasinvokefunctionurl4CA9917B - Impact: WILL_REPLACE
!!! If these destructive changes are necessary, please indicate this on the PR !!!

Links

https://github.com/aws/aws-cdk/tree/69cff2eda84dacfe265d518fc88ccf55930f59c4/packages/%40aws-cdk/integ-runner#update-workflow

[msambol]

@caevv Have you opened the PR yet? Can you link it?

[tim-finnigan]

The Update Workflow section you linked also notes:

By default, integration tests are run with the "update workflow" enabled. This can be disabled by using the --disable-update-workflow command line option

If an existing snapshot is being updated, the integration test runner will first deploy the existing snapshot and then perform a stack update with the new changes. This is to test for cases where an update would cause a breaking change only on a stack update.

That being said, maybe further clarification is needed on this part regarding the format/content of what the indication message should have:

If the destructive changes are expected (and required) then please indicate this on your PR.

[github-actions]

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

[scanlonp]

Hey @caevv, you can call it out in the PR body such as,

Destructive changes in the integ test: in integ.lambda.test.js, there were destructive changes on these resrouces: Lambda1, Lambda2. This is necessary because ... (the important reason).

Seeing that warning is often an indication that the code change is breaking and likely not the correct approach. If it is necessary, it should bejustified.

As reviewers, we want to be aware of potential breaking behavior. Calling out destructive changes alerts us to this possibility and let's us handle the code change with more care than normal. Still, there are cases where these changes are valid, which is why this text exists.

Hope this answers your question!

[caevv]

I'm just asking for clarification on what is needed here:

If the destructive changes are expected (and required) then please indicate this on your PR.

I managed to look into the repo and find that you need to add an allowDestroy flag.

[aws-cdk-automation]

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.