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

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Jun 11, 2019

Minimal representation of BuildSpec, formalizing some logic
that was already in Project but not really well-placed there.

It has very minimal support for merging buildspecs (only commands,
not artifacts or anything else).

Internal representation is hidden though, and can be improved and
extended later.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Minimal representation of BuildSpec, formalizing some logic
that was already in `Project` but not really well-placed there.

It has very minimal support for merging buildspecs (only commands,
not artifacts or anything else).

Internal representation is hidden though, and can be improved and
extended later.
@rix0rrr rix0rrr requested review from RomainMuller, skinny85 and a team as code owners June 11, 2019 11:45
*
* 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...

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

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Some minor comments, but in general looks good to me.

I wonder whether we could go a tiny step further, and provide another class with an API like:

BuildSpec.version02({
  preBuild: ['npm run install'],
  build: ['npm run build'],
  postBuild: ...
  artifacts: {
     baseDirectory: 'foo',
    files: ['**/*'],
  },
  secondaryArtifacts: {
    bar: {
      // ...
    },
  },
});

But I understand if you consider that to be "too big" for this PR.

*
* 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.

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

/**
* 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?

@@ -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?

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.

@rix0rrr rix0rrr merged commit 86a2192 into master Jun 12, 2019
@rix0rrr rix0rrr deleted the huijbers/codebuild-buildspec branch June 12, 2019 14:32
ScOut3R pushed a commit to ScOut3R/aws-cdk that referenced this pull request Jun 13, 2019
Minimal representation of BuildSpec, formalizing some logic
that was already in `Project` but not really well-placed there.

It has very minimal support for merging buildspecs (only commands,
not artifacts or anything else).

Internal representation is hidden though, and can be improved and
extended later.

BREAKING CHANGE:

* **codebuild**: buildSpec argument is now a `BuildSpec` object.
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants