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(codebuild): start representing BuildSpec as object #2820

Merged
merged 2 commits into from
Jun 12, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
101 changes: 101 additions & 0 deletions packages/@aws-cdk/aws-codebuild/lib/build-spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import { IResolveContext, Lazy, Stack } from '@aws-cdk/cdk';

/**
* BuildSpec for CodeBuild projects
*/
export abstract class BuildSpec {
public static fromObject(value: {[key: string]: any}): BuildSpec {
return new StructuredBuildSpec(value);
}

/**
* Use a file from the source as buildspec
*
* Use this if you want to use a file different from 'buildspec.yml'`
*/
public static fromFilename(filename: string): BuildSpec {
Copy link
Contributor

Choose a reason for hiding this comment

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

fromBuildSource(filePath: string)

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually like fromFilename a lot more, fromBuildSource is a weird name in my opinion...

return new FilenameBuildSpec(filename);
}

protected constructor() {
}

/**
* Render the represented BuildSpec
*/
public abstract toBuildSpec(): string;
}

/**
* BuildSpec that just returns the input unchanged
*/
class FilenameBuildSpec extends BuildSpec {
constructor(private readonly filename: string) {
super();
}

public toBuildSpec(): string {
return this.filename;
}

public toString() {
return `<buildspec file: ${this.filename}>`;
}
}

/**
* BuildSpec that understands about structure
*/
class StructuredBuildSpec extends BuildSpec {
Copy link
Contributor

Choose a reason for hiding this comment

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

This name is surprising to me... I would actually say this is the complete opposite of a StructuredBuildSpec - it's an UnstructuredBuildSpec, because I can say new StructuredBuildSpec({ foo: 123 }), and nothing would complain (until deployment, that is...).

If UnstructuredBuildSpec is too weird, maybe ObjectBuildSpec?

constructor(public readonly spec: {[key: string]: any}) {
super();
}

public toBuildSpec(): string {
// We have to pretty-print the buildspec, otherwise
// CodeBuild will not recognize it as an inline buildspec.
return Lazy.stringValue({ produce: (ctx: IResolveContext) =>
Stack.of(ctx.scope).toJsonString(this.spec, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this need to be lazy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we need access to stack.toJsonString.

CloudFormationLang.toJson() would have been good enough, but that's internal and hidden so I can't get to it.

});
}
}

/**
* Merge two buildspecs into a new BuildSpec
*
* NOTE: will currently only merge commands, not artifact
* declarations, environment variables, secrets, or any
* other configuration elements.
*
* Internal for now because it's not complete/good enough
* to expose on the objects directly, but we need to it to
* keep feature-parity for Project.
*
* @internal
*/
export function mergeBuildSpecs(lhs: BuildSpec, rhs: BuildSpec): BuildSpec {
if (!(lhs instanceof StructuredBuildSpec) || !(rhs instanceof StructuredBuildSpec)) {
throw new Error('Can only merge buildspecs created using BuildSpec.fromObject()');
}

return new StructuredBuildSpec(copyCommands(lhs.spec, rhs.spec));
}

/**
* Extend buildSpec phases with the contents of another one
*/
function copyCommands(buildSpec: any, extend: any): any {
if (buildSpec.version === '0.1') {
throw new Error('Cannot extend buildspec at version "0.1". Set the version to "0.2" or higher instead.');
}

const ret = Object.assign({}, buildSpec); // Return a copy
ret.phases = Object.assign({}, ret.phases);

for (const phaseName of Object.keys(extend.phases)) {
const phase = ret.phases[phaseName] = Object.assign({}, ret.phases[phaseName]);
phase.commands = [...phase.commands || [], ...extend.phases[phaseName].commands];
}

return ret;
}
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-codebuild/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export * from './project';
export * from './source';
export * from './artifacts';
export * from './cache';
export * from './build-spec';

// AWS::CodeBuild CloudFormation Resources:
export * from './codebuild.generated';
61 changes: 15 additions & 46 deletions packages/@aws-cdk/aws-codebuild/lib/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import iam = require('@aws-cdk/aws-iam');
import kms = require('@aws-cdk/aws-kms');
import { Aws, Construct, IResource, Lazy, PhysicalName, Resource, ResourceIdentifiers, Stack } from '@aws-cdk/cdk';
import { BuildArtifacts, CodePipelineBuildArtifacts, NoBuildArtifacts } from './artifacts';
import { BuildSpec, mergeBuildSpecsCommands } from './build-spec';
import { Cache } from './cache';
import { CfnProject } from './codebuild.generated';
import { BuildSource, NoSource, SourceType } from './source';
Expand Down Expand Up @@ -371,7 +372,7 @@ export interface CommonProjectProps {
*
* @default - Empty buildspec.
*/
readonly buildSpec?: any;
readonly buildSpec?: BuildSpec;

/**
* Run a script from an asset as build script
Expand Down Expand Up @@ -647,12 +648,14 @@ export class Project extends ProjectBase {

// Inject download commands for asset if requested
const environmentVariables = props.environmentVariables || {};
const buildSpec = props.buildSpec || {};
let buildSpec = props.buildSpec;

if (props.buildScriptAsset) {
environmentVariables[S3_BUCKET_ENV] = { value: props.buildScriptAsset.s3BucketName };
environmentVariables[S3_KEY_ENV] = { value: props.buildScriptAsset.s3ObjectKey };
extendBuildSpec(buildSpec, this.buildImage.runScriptBuildspec(props.buildScriptAssetEntrypoint || 'build.sh'));

const runScript = this.buildImage.runScriptBuildspec(props.buildScriptAssetEntrypoint || 'build.sh');
buildSpec = buildSpec ? mergeBuildSpecsCommands(buildSpec, runScript) : runScript;
props.buildScriptAsset.grantRead(this.role);
}

Expand All @@ -663,17 +666,10 @@ export class Project extends ProjectBase {
}

const sourceJson = this.source._toSourceJSON();
if (typeof buildSpec === 'string') {
return {
...sourceJson,
buildSpec // Filename to buildspec file
};
} else if (Object.keys(buildSpec).length > 0) {
// We have to pretty-print the buildspec, otherwise
// CodeBuild will not recognize it as an inline buildspec.
if (buildSpec) {
return {
...sourceJson,
buildSpec: JSON.stringify(buildSpec, undefined, 2)
buildSpec: buildSpec.toBuildSpec()
};
} else if (this.source.type === SourceType.None) {
throw new Error("If the Project's source is NoSource, you need to provide a buildSpec");
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an interesting edge case here. I think the actual behavior should be to error out also if no source has been provided, and the build spec has been given as BuildSpec.fromFileName(). I'm not sure what would be a good way to accomplish that - perhaps a public abstract validate(source: BuildSource): string[] method on the BuildSpec class?

It's not super crucial though.

Expand Down Expand Up @@ -1025,7 +1021,7 @@ export interface IBuildImage {
/**
* Make a buildspec to run the indicated script
*/
runScriptBuildspec(entrypoint: string): any;
runScriptBuildspec(entrypoint: string): BuildSpec;
}

/**
Expand Down Expand Up @@ -1124,8 +1120,8 @@ export class LinuxBuildImage implements IBuildImage {
return [];
}

public runScriptBuildspec(entrypoint: string): any {
return {
public runScriptBuildspec(entrypoint: string): BuildSpec {
return BuildSpec.fromObject({
version: '0.2',
phases: {
pre_build: {
Expand All @@ -1149,7 +1145,7 @@ export class LinuxBuildImage implements IBuildImage {
]
}
}
};
});
}
}

Expand Down Expand Up @@ -1220,8 +1216,8 @@ export class WindowsBuildImage implements IBuildImage {
return ret;
}

public runScriptBuildspec(entrypoint: string): any {
return {
public runScriptBuildspec(entrypoint: string): BuildSpec {
return BuildSpec.fromObject({
version: '0.2',
phases: {
pre_build: {
Expand All @@ -1242,7 +1238,7 @@ export class WindowsBuildImage implements IBuildImage {
]
}
}
};
});
}
}

Expand Down Expand Up @@ -1272,33 +1268,6 @@ export enum BuildEnvironmentVariableType {
ParameterStore = 'PARAMETER_STORE'
}

/**
* Extend buildSpec phases with the contents of another one
*/
function extendBuildSpec(buildSpec: any, extend: any) {
if (typeof buildSpec === 'string') {
throw new Error('Cannot extend buildspec that is given as a string. Pass the buildspec as a structure instead.');
}
if (buildSpec.version === '0.1') {
throw new Error('Cannot extend buildspec at version "0.1". Set the version to "0.2" or higher instead.');
}
if (buildSpec.version === undefined) {
buildSpec.version = extend.version;
}

if (!buildSpec.phases) {
buildSpec.phases = {};
}

for (const phaseName of Object.keys(extend.phases)) {
if (!(phaseName in buildSpec.phases)) { buildSpec.phases[phaseName] = {}; }
const phase = buildSpec.phases[phaseName];

if (!(phase.commands)) { phase.commands = []; }
phase.commands.push(...extend.phases[phaseName].commands);
}
}

function ecrAccessForCodeBuildService(): iam.PolicyStatement {
return new iam.PolicyStatement()
.describe('CodeBuild')
Expand Down
5 changes: 3 additions & 2 deletions packages/@aws-cdk/cdk/lib/cloudformation-lang.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ export class CloudFormationLang {
* in CloudFormation will fail.
*
* @param obj The object to stringify
* @param number Indentation to use
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant @param space here...? Also, what's the default?

*/
public static toJSON(obj: any): string {
public static toJSON(obj: any, space?: number): string {
// This works in two stages:
//
// First, resolve everything. This gets rid of the lazy evaluations, evaluation
Expand Down Expand Up @@ -61,7 +62,7 @@ export class CloudFormationLang {
JSON.stringify(resolve(obj, {
scope: ctx.scope,
resolver: new IntrinsincWrapper()
}))
}), undefined, space)
});

function wrap(value: any): any {
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/cdk/lib/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,8 @@ export class Stack extends Construct implements ITaggable {
/**
* Convert an object, potentially containing tokens, to a JSON string
*/
public toJsonString(obj: any): string {
return CloudFormationLang.toJSON(obj).toString();
public toJsonString(obj: any, space?: number): string {
return CloudFormationLang.toJSON(obj, space).toString();
}

/**
Expand Down