Skip to content
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

fix(pipelines): changing synth action doesn't restart pipeline #10176

Merged
merged 8 commits into from
Sep 10, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions packages/@aws-cdk/assert/lib/assertions/have-resource-matchers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,90 @@ export function notMatching(matcher: any): PropertyMatcher {
});
}

export type TypeValidator<T> = (x: any) => x is T;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add corresponding entries in the README for these new features


/**
* Captures a value onto an object if it matches a given inner matcher
*
* @example
*
* const someValue = Capture.aString();
* expect(stack).toHaveResource({
* // ...
* Value: someValue.capture(stringMatching('*a*')),
* });
* console.log(someValue.capturedValue);
*/
export class Capture<T=any> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me.

We'll definitely consider for this to be ported. Just wanted to make note that the form might have to change.

/**
* A Capture object that captures any type
*/
public static anyType(): Capture<any> {
return new Capture();
}

/**
* A Capture object that captures a custom type
*/
public static aString(): Capture<string> {
return new Capture((x: any): x is string => {
if (typeof x !== 'string') {
throw new Error(`Expected to capture a string, got '${x}'`);
}
return true;
});
}

/**
* A Capture object that captures a custom type
*/
public static a<T>(validator: TypeValidator<T>): Capture<T> {
return new Capture(validator);
}

private _value?: T;
private _didCapture = false;
private _wasInvoked = false;

protected constructor(private readonly typeValidator?: TypeValidator<T>) {
}

/**
* Capture the value if the inner matcher successfully matches it
*
* If no matcher is given, `anything()` is assumed.
*/
public capture(matcher?: any): PropertyMatcher {
if (matcher === undefined) {
matcher = anything();
}

return annotateMatcher({ $capture: matcher }, (value: any, failure: InspectionFailure) => {
this._wasInvoked = true;
const result = matcherFrom(matcher)(value, failure);
if (result) {
if (this.typeValidator && !this.typeValidator(value)) {
throw new Error(`Value not of the expected type: ${value}`);
}
this._didCapture = true;
this._value = value;
}
return result;
});
}

public get didCapture() {
return this._didCapture;
}

public get capturedValue(): T {
if (!this.didCapture) {
throw new Error(`Did not capture a value: ${this._wasInvoked ? 'inner matcher failed' : 'never invoked'}`);
}
return this._value!;
}
}

/**
* Match on the innards of a JSON string, instead of the complete string
*/
Expand Down
15 changes: 14 additions & 1 deletion packages/@aws-cdk/assert/test/have-resource.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { writeFileSync } from 'fs';
import { join } from 'path';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import * as cxapi from '@aws-cdk/cx-api';
import { ABSENT, arrayWith, exactValue, expect as cdkExpect, haveResource, haveResourceLike } from '../lib/index';
import { ABSENT, arrayWith, exactValue, expect as cdkExpect, haveResource, haveResourceLike, Capture, anything } from '../lib/index';

test('support resource with no properties', () => {
const synthStack = mkStack({
Expand Down Expand Up @@ -238,6 +238,19 @@ describe('property absence', () => {
}));
}).not.toThrowError();
});

test('test capturing', () => {
const synthStack = mkSomeResource({
Prop: 'somevalue',
});

const propValue = Capture.aString();
cdkExpect(synthStack).to(haveResourceLike('Some::Resource', {
Prop: propValue.capture(anything()),
}));

expect(propValue.capturedValue).toEqual('somevalue');
});
});

function mkStack(template: any): cxapi.CloudFormationStackArtifact {
Expand Down
77 changes: 53 additions & 24 deletions packages/@aws-cdk/pipelines/lib/synths/simple-synth-action.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as crypto from 'crypto';
import * as path from 'path';
import * as codebuild from '@aws-cdk/aws-codebuild';
import * as codepipeline from '@aws-cdk/aws-codepipeline';
Expand Down Expand Up @@ -262,32 +263,47 @@ export class SimpleSynthAction implements codepipeline.IAction {
const testCommands = this.props.testCommands ?? [];
const synthCommand = this.props.synthCommand;

const project = new codebuild.PipelineProject(scope, 'CdkBuildProject', {
projectName: this.props.projectName ?? this.props.projectName,
environment: { buildImage: codebuild.LinuxBuildImage.STANDARD_4_0, ...this.props.environment },
buildSpec: codebuild.BuildSpec.fromObject({
version: '0.2',
phases: {
pre_build: {
commands: filterEmpty([
this.props.subdirectory ? `cd ${this.props.subdirectory}` : '',
...installCommands,
]),
},
build: {
commands: filterEmpty([
...buildCommands,
...testCommands,
synthCommand,
]),
},
const buildSpec = codebuild.BuildSpec.fromObject({
version: '0.2',
phases: {
pre_build: {
commands: filterEmpty([
this.props.subdirectory ? `cd ${this.props.subdirectory}` : '',
...installCommands,
]),
},
build: {
commands: filterEmpty([
...buildCommands,
...testCommands,
synthCommand,
]),
},
artifacts: renderArtifacts(this),
}),
environmentVariables: {
...copyEnvironmentVariables(...this.props.copyEnvironmentVariables || []),
...this.props.environmentVariables,
},
artifacts: renderArtifacts(this),
});

const environment = { buildImage: codebuild.LinuxBuildImage.STANDARD_4_0, ...this.props.environment };

const environmentVariables = {
...copyEnvironmentVariables(...this.props.copyEnvironmentVariables || []),
...this.props.environmentVariables,
};

// A hash over the values that make the CodeBuild Project unique (and necessary
// to restart the pipeline if one of them changes). projectName is not necessary to include
// here because the pipeline will definitely restart if projectName changes.
const projectConfigHash = hash({
environment,
buildSpecString: buildSpec.toBuildSpec(),
environmentVariables,
});

const project = new codebuild.PipelineProject(scope, 'CdkBuildProject', {
projectName: this.props.projectName ?? this.props.projectName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

environment,
buildSpec,
environmentVariables,
});

if (this.props.rolePolicyStatements !== undefined) {
Expand All @@ -300,6 +316,13 @@ export class SimpleSynthAction implements codepipeline.IAction {
actionName: this.actionProperties.actionName,
input: this.props.sourceArtifact,
outputs: [this.props.cloudAssemblyArtifact, ...(this.props.additionalArtifacts ?? []).map(a => a.artifact)],

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not just the buildspec but anything around the CodeBuild project.

Suggested change
// 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.

environmentVariables: {
_PROJECT_CONFIG_HASH: { value: projectConfigHash },
},
project,
});
this._actionProperties = this._action.actionProperties;
Expand Down Expand Up @@ -411,4 +434,10 @@ export interface StandardYarnSynthOptions extends SimpleSynthOptions {
* @default 'npx cdk synth'
*/
readonly synthCommand?: string;
}

function hash<A>(obj: A) {
const d = crypto.createHash('sha256');
d.update(JSON.stringify(obj));
return d.digest('hex');
}
72 changes: 71 additions & 1 deletion packages/@aws-cdk/pipelines/test/builds.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { arrayWith, deepObjectLike, encodedJson } from '@aws-cdk/assert';
import { arrayWith, deepObjectLike, encodedJson, objectLike, Capture } from '@aws-cdk/assert';
import '@aws-cdk/assert/jest';
import * as cbuild from '@aws-cdk/aws-codebuild';
import * as codepipeline from '@aws-cdk/aws-codepipeline';
import { Stack } from '@aws-cdk/core';
import * as cdkp from '../lib';
Expand Down Expand Up @@ -219,6 +220,75 @@ test('Standard (NPM) synth can output additional artifacts', () => {
});
});

test('Pipeline action contains a hash that changes as the buildspec changes', () => {
const hash1 = synthWithAction((sa, cxa) => cdkp.SimpleSynthAction.standardNpmSynth({
sourceArtifact: sa,
cloudAssemblyArtifact: cxa,
}));
const hash2 = synthWithAction((sa, cxa) => cdkp.SimpleSynthAction.standardNpmSynth({
sourceArtifact: sa,
cloudAssemblyArtifact: cxa,
installCommand: 'do install',
}));
const hash3 = synthWithAction((sa, cxa) => cdkp.SimpleSynthAction.standardNpmSynth({
sourceArtifact: sa,
cloudAssemblyArtifact: cxa,
environment: {
computeType: cbuild.ComputeType.LARGE,
},
}));
const hash4 = synthWithAction((sa, cxa) => cdkp.SimpleSynthAction.standardNpmSynth({
sourceArtifact: sa,
cloudAssemblyArtifact: cxa,
environmentVariables: {
xyz: { value: 'SOME-VALUE' },
},
}));

expect(hash1).not.toEqual(hash2);
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
expect(hash1).not.toEqual(hash3);
expect(hash1).not.toEqual(hash4);
expect(hash2).not.toEqual(hash3);
expect(hash2).not.toEqual(hash4);
expect(hash3).not.toEqual(hash4);

function synthWithAction(cb: (sourceArtifact: codepipeline.Artifact, cloudAssemblyArtifact: codepipeline.Artifact) => codepipeline.IAction) {
const _app = new TestApp({ outdir: 'testcdk.out' });
const _pipelineStack = new Stack(_app, 'PipelineStack', { env: PIPELINE_ENV });
const _sourceArtifact = new codepipeline.Artifact();
const _cloudAssemblyArtifact = new codepipeline.Artifact('CloudAsm');

new TestGitHubNpmPipeline(_pipelineStack, 'Cdk', {
sourceArtifact: _sourceArtifact,
cloudAssemblyArtifact: _cloudAssemblyArtifact,
synthAction: cb(_sourceArtifact, _cloudAssemblyArtifact),
});

const theHash = Capture.aString();
expect(_pipelineStack).toHaveResourceLike('AWS::CodePipeline::Pipeline', {
Stages: arrayWith({
Name: 'Build',
Actions: [
objectLike({
Name: 'Synth',
Configuration: objectLike({
EnvironmentVariables: encodedJson([
{
name: '_PROJECT_CONFIG_HASH',
type: 'PLAINTEXT',
value: theHash.capture(),
},
]),
}),
}),
],
}),
});

return theHash.capturedValue;
}
});

function npmYarnBuild(npmYarn: string) {
if (npmYarn === 'npm') { return cdkp.SimpleSynthAction.standardNpmSynth; }
if (npmYarn === 'yarn') { return cdkp.SimpleSynthAction.standardYarnSynth; }
Expand Down
Loading