fix(codebuild): Fixed build spec file format to return yaml #13445

janario · 2021-03-06T15:19:33Z

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

janario · 2021-03-06T15:26:04Z

Hi 👋

First of all, I wanted to check what breaking compatibilities this could have.

I'm using amplify and as it recommends I create my build file using the codebuild class. But I noticed that it is creating a json instead of a yaml, it works fine but I would prefer to keep it consistent

json:

yaml (after):

CodeBuild: I'm not familiar with code-build, but in the console, it seems to work with yaml, could you confirm if this can break somehow?

gitpod-io · 2021-03-06T15:38:21Z

skinny85

Thanks for the contribution @janario !

Your build is failing on linter rules:

@aws-cdk/aws-codebuild: /codebuild/output/src641264259/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-codebuild/lib/build-spec.ts
@aws-cdk/aws-codebuild:   2:8   error  A space is required after '{'   object-curly-spacing
@aws-cdk/aws-codebuild:   2:15  error  A space is required before '}'  object-curly-spacing
@aws-cdk/aws-codebuild:   2:22  error  Strings must use singlequote    quotes
@aws-cdk/aws-codebuild: âœ– 3 problems (3 errors, 0 warnings)
@aws-cdk/aws-codebuild:   3 errors and 0 warnings potentially fixable with the `--fix` option.
@aws-cdk/aws-codebuild: Error: /codebuild/output/src641264259/src/github.com/aws/aws-cdk/node_modules/eslint/bin/eslint.js . --ext=.ts --resolve-plugins-relative-to=/codebuild/output/src641264259/src/github.com/aws/aws-cdk/tools/cdk-build-tools/lib exited with error code 1
@aws-cdk/aws-codebuild: Build failed.!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

(You can see the log of your build in a comment submitted to the PR, like this one: #13445 (comment) ).

May I suggest something? Instead of changing the existing fromObject() method, that will result in many, many changes to existing templates (both inside this repository in the various tests, and also for customers of the CodeBuild library), how about you create a new static factory method on the BuildSpec class, called something like yamlFromObject(), and make only that one result in rendering YAML? You can then describe in the ReadMe of the package how to use that new method.

Thoughts on this idea?

Thanks,
Adam

packages/@aws-cdk/aws-codebuild/lib/build-spec.ts

janario · 2021-03-09T08:06:45Z

Thanks for the contribution @janario !

Your build is failing on linter rules:
@aws-cdk/aws-codebuild: /codebuild/output/src641264259/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-codebuild/lib/build-spec.ts
@aws-cdk/aws-codebuild:   2:8   error  A space is required after '{'   object-curly-spacing
@aws-cdk/aws-codebuild:   2:15  error  A space is required before '}'  object-curly-spacing
@aws-cdk/aws-codebuild:   2:22  error  Strings must use singlequote    quotes
@aws-cdk/aws-codebuild: âœ– 3 problems (3 errors, 0 warnings)
@aws-cdk/aws-codebuild:   3 errors and 0 warnings potentially fixable with the `--fix` option.
@aws-cdk/aws-codebuild: Error: /codebuild/output/src641264259/src/github.com/aws/aws-cdk/node_modules/eslint/bin/eslint.js . --ext=.ts --resolve-plugins-relative-to=/codebuild/output/src641264259/src/github.com/aws/aws-cdk/tools/cdk-build-tools/lib exited with error code 1
@aws-cdk/aws-codebuild: Build failed.!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
(You can see the log of your build in a comment submitted to the PR, like this one: #13445 (comment) ).

May I suggest something? Instead of changing the existing fromObject() method, that will result in many, many changes to existing templates (both inside this repository in the various tests, and also for customers of the CodeBuild library), how about you create a new static factory method on the BuildSpec class, called something like yamlFromObject(), and make only that one result in rendering YAML? You can then describe in the ReadMe of the package how to use that new method.

Thoughts on this idea?

Thanks,
Adam

Perfect, yeah I realized that many test would be broke and compatibility could be affected, I wanted to bring for discussion first
#13445 (comment)

I'll try to create a new yamlFromObject and see how it goes, will update this PR soon, for now changing it to draft

Pull request has been modified.

janario · 2021-03-13T14:08:52Z

@skinny85 Changed to be a new method and added some test, as well as changed the doc of amplify.

Let me know your opinion 👍

skinny85

Looks great @janario!

A few minor comments. I apologize for the churn of the name of this method, but I think we can come up with something better than yamlFromObject (which I didn't spend enough time thinking on before suggesting it, for which I'm sorry).

skinny85 · 2021-03-15T22:09:58Z

packages/@aws-cdk/aws-amplify/README.md

@@ -38,7 +38,7 @@ const amplifyApp = new amplify.App(this, 'MyApp', {
    repository: '<repo>',
    oauthToken: cdk.SecretValue.secretsManager('my-github-token')
  }),
-  buildSpec: codebuild.BuildSpec.fromObject({ // Alternatively add a `amplify.yml` to the repo
+  buildSpec: codebuild.BuildSpec.yamlFromObject({ // Alternatively add a `amplify.yml` to the repo


We also need to update the ReadMe of the CodeBuild module.

I've updated the readme, but here I have some doubts

I'm not familiar with codebuild, so they might be a bit stupid :-) .

Is buildSpec from codebuild expected to be a yaml?

The old json function goes over until the function tokenAwareStringify.

Is there something that codebuild would expect from this token function that the yaml would not cover yet? Not sure if it is more general for cloudformation or if codebuild needs something from it.

Stack.of(ctx.scope).toJsonString(this.spec, 2), > CloudFormationLang.toJSON(obj, space).toString(); >> tokenAwareStringify()

packages/@aws-cdk/aws-amplify/test/app.test.ts

packages/@aws-cdk/aws-codebuild/lib/build-spec.ts

packages/@aws-cdk/aws-codebuild/test/test.project.ts

Pull request has been modified.

skinny85

@janario this looks fantastic! I have one last comment to the ReadMe, and then we can merge this in!

Thanks for the contribution!

skinny85 · 2021-03-18T00:11:37Z

packages/@aws-cdk/aws-codebuild/README.md

@@ -583,7 +583,7 @@ For example:

 ```ts
 new codebuild.Project(stack, 'MyProject', {
-  buildSpec: codebuild.BuildSpec.fromObject({
+  buildSpec: codebuild.BuildSpec.fromObjectToYaml({


Instead of changing these, can you just add a paragraph in the ReadMe saying something like "If you'd prefer your buildspec to be rendered as YAML in the template, use the fromObjectToYaml() method instead", and a simple example snippet of code?

Pull request has been modified.

…es to use yaml format

skinny85

Awesome work @janario, thanks so much for the contribution!

mergify · 2021-03-22T15:15:46Z

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-cdk-automation · 2021-03-22T15:49:52Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 333fefc
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-03-22T15:50:09Z

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

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

github-actions bot added the @aws-cdk/aws-codebuild Related to AWS CodeBuild label Mar 6, 2021

github-actions bot assigned skinny85 Mar 6, 2021

skinny85 previously requested changes Mar 8, 2021

View reviewed changes

packages/@aws-cdk/aws-codebuild/lib/build-spec.ts Outdated Show resolved Hide resolved

janario marked this pull request as draft March 9, 2021 08:06

janario force-pushed the fix/build-spec-yaml-format branch from ce45f8e to 2ec91bc Compare March 13, 2021 13:29

janario marked this pull request as ready for review March 13, 2021 14:07

skinny85 previously requested changes Mar 15, 2021

View reviewed changes

skinny85 previously requested changes Mar 18, 2021

View reviewed changes

janario added 4 commits March 21, 2021 10:13

Added yamlFromObject BuildSpec

39989bc

Changed documentation of amplify to use yaml object. Changed test cas…

320d8ff

…es to use yaml format

Code review adjustments

4cca189

Reverted doc and added info to render buildspec as YAML

cbaefe1

janario force-pushed the fix/build-spec-yaml-format branch from 9cac563 to cbaefe1 Compare March 21, 2021 09:13

janario and others added 2 commits March 21, 2021 10:36

Reverted doc

a5ecb9f

Add a line break in the CodeBuild ReadMe

3497dea

skinny85 approved these changes Mar 22, 2021

View reviewed changes

Merge branch 'master' into fix/build-spec-yaml-format

333fefc

mergify bot merged commit fab93c6 into aws:master Mar 22, 2021

eladb pushed a commit that referenced this pull request Mar 24, 2021

fix(codebuild): Fixed build spec file format to return yaml (#13445)

a710dc7

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

GraemeBradbury mentioned this pull request Mar 25, 2021

‼️ NOTICE: "Cannot use import statement outside a module" in CodeBuild in 1.95.0 #13795

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(codebuild): Fixed build spec file format to return yaml #13445

fix(codebuild): Fixed build spec file format to return yaml #13445

janario commented Mar 6, 2021

janario commented Mar 6, 2021

gitpod-io bot commented Mar 6, 2021 •

edited

Loading

skinny85 left a comment

janario commented Mar 9, 2021

janario commented Mar 13, 2021

skinny85 left a comment

skinny85 Mar 15, 2021

janario Mar 17, 2021

skinny85 left a comment

skinny85 Mar 18, 2021 •

edited

Loading

janario Mar 21, 2021

skinny85 left a comment

mergify bot commented Mar 22, 2021

aws-cdk-automation commented Mar 22, 2021

mergify bot commented Mar 22, 2021

fix(codebuild): Fixed build spec file format to return yaml #13445

fix(codebuild): Fixed build spec file format to return yaml #13445

Conversation

janario commented Mar 6, 2021

janario commented Mar 6, 2021

gitpod-io bot commented Mar 6, 2021 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

janario commented Mar 9, 2021

janario commented Mar 13, 2021

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Mar 15, 2021

Choose a reason for hiding this comment

janario Mar 17, 2021

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

janario Mar 21, 2021

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 22, 2021

aws-cdk-automation commented Mar 22, 2021

AWS CodeBuild CI Report

mergify bot commented Mar 22, 2021

gitpod-io bot commented Mar 6, 2021 •

edited

Loading

skinny85 Mar 18, 2021 •

edited

Loading