fix(pipelines): changing synth action doesn't restart pipeline #10176
Conversation
Pipeline structure looks like this: ``` ┌────────┐ ┌────────┐ ┌──────────┐ │ │ │ │ │ │ │ src │───────▶│ synth │───────▶│selfupdate│─────▶...(deploy)... │ │ │ │ │ │ └────────┘ └────────┘ └──────────┘ ``` When changing the pipeline structure, we expect it to restart (and it does). However, when changing something about the "synth" step (such as adding tests to the step), we would ALSO expect the pipeline to restart. This would ensure that if we add tests, the tests get executed and must pass before we start the actual deployments. Previously that didn't happen, because doing something like adding a test didn't actually change the *Pipeline structure* (causing a restart). Instead, it would change the buildspec of the CodeBuild Project the pipeline was referencing, leaving the pipeline unchanged and ready to continue deploying. The solution is a hash of the buildspec somewhere, but where? We could have put it in the CodeBuild Project's logical id, so that we would have torn down and created a new CodeBuild project to ensure that the pipeline would restart. However, doing so would prevent giving the CodeBuild project a deterministic display name (replacement would fail). Plus, a simpler and cheaper solution is to put a hash of the buildspec in the Action that references the CodeBuild project. There is exactly one field where we can stash some user-specified data that doesn't impact the build result: `EnvironmentVariables`. We add an environment variable with an unlikely name, which is not used by the build itself. Its sole purpose is to make the pipeline definition change when the buildspec changes, causing a pipeline restart. Fixes #9458.
| }); | ||
|
|
||
| const project = new codebuild.PipelineProject(scope, 'CdkBuildProject', { | ||
| projectName: this.props.projectName ?? this.props.projectName, |
| // Inclusion of the hash here will lead to the pipeline structure changing if the | ||
| // buildspec changes, and hence the pipeline being restarted. This is necessary if | ||
| // the users adds (for example) build or test commands to the buildspec. |
There was a problem hiding this comment.
Not just the buildspec but anything around the CodeBuild project.
| // Inclusion of the hash here will lead to the pipeline structure changing if the | |
| // buildspec changes, and hence the pipeline being restarted. This is necessary if | |
| // the users adds (for example) build or test commands to the buildspec. | |
| // Inclusion of the hash here will lead to the pipeline structure changing for any | |
| // changes to the underlying CodeBuild project, and hence the pipeline being restarted. | |
| // This is necessary if the users adds (for example) build or test commands to the buildspec. |
| @@ -281,6 +281,90 @@ export function notMatching(matcher: any): PropertyMatcher { | |||
| }); | |||
| } | |||
|
|
|||
| export type TypeValidator<T> = (x: any) => x is T; | |||
There was a problem hiding this comment.
Add corresponding entries in the README for these new features
| * }); | ||
| * console.log(someValue.capturedValue); | ||
| */ | ||
| export class Capture<T=any> { |
There was a problem hiding this comment.
sigh one more thing to consider when rewriting this module as a JSII-able module.
I would've asked - why not just
const notThisHash = ...;
expect(stack).not.toHaveResourceLike('AWS::CodeBuild::Project', {
...
Value: notThisHash
});But looking at the usage and the deep nesting, I understand why you had to do this.
There was a problem hiding this comment.
The functionality is legit and should be ported, I feel.
The jsii-able version of this might drop the type assertions but I didn't see any point in having this wobbly-typed since this module is long past jsii-ability anyway.
There was a problem hiding this comment.
Fine by me.
We'll definitely consider for this to be ported. Just wanted to make note that the form might have to change.
|
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). |
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Pipeline structure looks like this:
When changing the pipeline structure, we expect it to restart (and it
does).
However, when changing something about the "synth" action (such as
adding tests to the list of commands), we would ALSO expect the pipeline to restart.
This would ensure that if we add tests, the tests get executed and
must pass before we start the actual deployments. That is, we'd like
to run the tests on the CURRENT commit as well, but by the time
we update the buildspec to run the tests, we've already passed the stage
where the tests would be run.
This is because doing something like adding
a test didn't actually change the Pipeline structure (causing
a restart). Instead, it would change the buildspec of the
CodeBuild Project the pipeline was referencing, leaving the pipeline
unchanged and ready to continue deploying. The tests would
only be executed for FUTURE commits, leaving the current commit
untested.
The solution is a hash of the buildspec somewhere, but where?
We could have put it in the CodeBuild Project's logical id, so that we
would have torn down and created a new CodeBuild project to ensure that
the pipeline would restart. However, doing so would prevent giving the
CodeBuild project a deterministic display name (replacement would fail).
Plus, a simpler and cheaper solution is to put a hash of the buildspec
in the Action that references the CodeBuild project.
There is exactly one field where we can stash some user-specified
data that doesn't impact the build result:
EnvironmentVariables.We add an environment variable with an unlikely name, which is not
used by the build itself. Its sole purpose is to make the pipeline
definition change when the buildspec changes, causing a pipeline
restart.
Fixes #9458.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license