-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix(pipelines): prevent self-mutation on asset updates #9183
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
Conversation
Currently, changes to the asset hashes of content results in changes to the definitions of the pipeline, causing the self-mutate state to trigger. This can cause infinite loops for systems where build artifacts change on each build. The cause for this looping is that the asset hash was embedded as the pipeline action name. By replacing the asset hash with a simple asset counter (already used for the action CloudFormation id), the pipeline definition should remain stable on asset changes. _Testing:_ Due to the pipeline executing the current version of CDK within the pipeline, it's impossible to actual verify that this corrects the issue. To test + verify, created a sample app where the asset hash changed on each build. Then ran subsequent 'cdk synth' commands and diff'ed the output. Prior to this change, there were numerous changes related to the asset hash in the pipeline stack. After, only the buildspec of the CodeBuild project that referenced the hash changed. This does not change the definition of the CodePipeline itself.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Beautiful!
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
@@ -279,7 +279,7 @@ class AssetPublishing extends Construct { | |||
const id = command.assetType === AssetType.FILE ? `FileAsset${this._fileAssetCtr++}` : `DockerAsset${this._dockerAssetCtr++}`; | |||
|
|||
action = this.publishers[command.assetId] = new PublishAssetsAction(this, id, { | |||
actionName: command.assetId, | |||
actionName: id, |
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.
Add a comment explaining why we are using the ID for the action name
Short question: Imagine the case where a new asset is added to a CDK app and all other assets remained unchanged (same hash as before). This PR leads to all assets having to be published again (in the worst case), as assets might get assigned to a different publishing action as before. E.g. if the newly added asset gets assigned to publishing action No. 1 and the unchanged assets get "shifted" by one publishing action. This might be a trade-off to accept though. Thanks for this PR @njlynch! |
) Follow-on to #9183. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Since the asset hash of all other assets would not change, the publisher ( |
Currently, changes to the asset hashes of content results in changes to the definitions of the pipeline, causing the self-mutate state to trigger. This can cause infinite loops for systems where build artifacts change on each build. The cause for this looping is that the asset hash was embedded as the pipeline action name. By replacing the asset hash with a simple asset counter (already used for the action CloudFormation id), the pipeline definition should remain stable on asset changes. _Testing:_ Due to the pipeline executing the current version of CDK within the pipeline, it's impossible to actual verify that this corrects the issue. To test + verify, created a sample app where the asset hash changed on each build. Then ran subsequent 'cdk synth' commands and diff'ed the output. Prior to this change, there were numerous changes related to the asset hash in the pipeline stack. After, only the buildspec of the CodeBuild project that referenced the hash changed. This does not change the definition of the CodePipeline itself. fixes aws#9080 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Currently, changes to the asset hashes of content results in changes to the definitions of the pipeline, causing the self-mutate state to trigger. This can cause infinite loops for systems where build artifacts change on each build. The cause for this looping is that the asset hash was embedded as the pipeline action name. By replacing the asset hash with a simple asset counter (already used for the action CloudFormation id), the pipeline definition should remain stable on asset changes. _Testing:_ Due to the pipeline executing the current version of CDK within the pipeline, it's impossible to actual verify that this corrects the issue. To test + verify, created a sample app where the asset hash changed on each build. Then ran subsequent 'cdk synth' commands and diff'ed the output. Prior to this change, there were numerous changes related to the asset hash in the pipeline stack. After, only the buildspec of the CodeBuild project that referenced the hash changed. This does not change the definition of the CodePipeline itself. fixes aws#9080 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Currently, changes to the asset hashes of content results in changes to the
definitions of the pipeline, causing the self-mutate state to trigger. This can
cause infinite loops for systems where build artifacts change on each build.
The cause for this looping is that the asset hash was embedded as the pipeline
action name. By replacing the asset hash with a simple asset counter (already
used for the action CloudFormation id), the pipeline definition should remain
stable on asset changes.
Testing: Due to the pipeline executing the current version of CDK within the
pipeline, it's impossible to actual verify that this corrects the issue. To
test + verify, created a sample app where the asset hash changed on each build.
Then ran subsequent 'cdk synth' commands and diff'ed the output. Prior to this
change, there were numerous changes related to the asset hash in the pipeline
stack. After, only the buildspec of the CodeBuild project that referenced the
hash changed. This does not change the definition of the CodePipeline itself.
fixes #9080
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license