feat(manifest): support codebuild environment image config #2125

tam0ri · 2021-04-01T15:46:53Z

Addresses #2106

pipeline init behavior
- generates manifest with build property automatically.
pipeline update behavior
- if build property is not specified in pipeline manifest, default image is used.
- if build property is specified explicitly in pipeline manifest, specified image is used.
Limitation: following images are descoped.
- cross-account ECR repository
- private registry other than ECR

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

* pipeline init generates with build property automatically. * if build property is not specified in pipeline manifest, default image is used. * if build property is specified explicitly in pipeline manifest, specified image is used. * Following images are descoped. ** cross-account ECR repository ** private registry other than ECR

efekarakus

Thank you so much for adding this feature! It looks great, just a few recommendations

efekarakus · 2021-04-01T23:48:16Z

internal/pkg/cli/pipeline_update.go

@@ -272,6 +274,7 @@ func (o *updatePipelineOpts) Execute() error {
 		AppName:         o.appName,
 		Name:            pipeline.Name,
 		Source:          source,
+		Build:           build,


nit: to remove the extra variable

Suggested change

Build: build,

Build: deploy.PipelineBuildFromManifest(pipeline.Build),

efekarakus · 2021-04-01T23:53:47Z

internal/pkg/manifest/pipeline.go

@@ -168,6 +170,11 @@ type Source struct {
 	Properties   map[string]interface{} `yaml:"properties"`
 }

+// Build defines the build project to build and test image.
+type Build struct {
+	Image string                 `yaml:"image"`


nit: is this formatted correctly with gofmt?

efekarakus · 2021-04-01T23:58:02Z

templates/cicd/pipeline.yml

+# This section defines the build project.
+build:
+  # The URI that identifies the Docker image to use for this build project. 
+  image: {{.Build.Image}}


I think we should not show this setting by default as it is mostly for advanced users. I expect most folks are satisfied with the default image that we provide.

Can we instead add it as a field in our documentation here? https://github.com/aws/copilot-cli/blob/mainline/site/content/docs/manifest/pipeline.md 🙏

efekarakus · 2021-04-02T00:01:01Z

internal/pkg/manifest/pipeline.go

@@ -17,6 +17,7 @@ const (
 	GithubV1ProviderName   = "GitHubV1"
 	CodeCommitProviderName = "CodeCommit"
 	BitbucketProviderName  = "Bitbucket"
+	DefaultImage           = "aws/codebuild/amazonlinux2-x86_64-standard:3.0"


Can we define this constant in thedeploy pkg instead? this would allow us to keep it private

efekarakus · 2021-04-02T00:02:12Z

internal/pkg/manifest/pipeline.go

+		Build: &Build{
+			Image: DefaultImage,
+		},


If leave the default to be nil, this allow us to move the constant to the deploy pkg and the translation logic to deploy.PipelineBuildFromManifest

efekarakus · 2021-04-02T00:06:17Z

internal/pkg/deploy/pipeline.go

+	var imageURI string
+	if (mfBuild == nil || mfBuild.Image == "") {
+		imageURI = manifest.DefaultImage
+	} else {
+		imageURI = mfBuild.Image
+	}
+	return &Build{
+		Image: imageURI,
+	}


Suggested change

var imageURI string

if (mfBuild == nil || mfBuild.Image == "") {

imageURI = manifest.DefaultImage

} else {

imageURI = mfBuild.Image

}

return &Build{

Image: imageURI,

}

image := defaultPipelineBuildImage

if mfBuild != nil && mfBuild.Image != "" {

image = mfBuild.Image

}

return &Build{

Image: image,

}

tam0ri · 2021-04-02T16:43:37Z

Thank you for your review and feedback. It was very helpful! I totally agreed your suggestions.

efekarakus

Looks great thank you! Just a small ask to remove the info block, afterwards we can remove the "do-not-merge" label and the PR will auto-merge :D

efekarakus · 2021-04-05T16:54:56Z

internal/pkg/deploy/cloudformation/pipeline_test.go

@@ -15,6 +15,8 @@ import (
 	"github.com/stretchr/testify/require"
 )

+const defaultImage = "aws/codebuild/amazonlinux2-x86_64-standard:3.0"


nit: can we reuse defaultPipelineBuildImage?

Sorry, I want to clarify. Do you mean rename defaultImage into defaultPipelineBuildImage? Or, using defaultPipelineBuildImage in internal/pkg/deploy/pipeline.go? This constant is in deploy pkg, but this test file is in cloudformation pkg. Do you intend to fix not internal/pkg/deploy/cloudformation/pipeline_test.go but l.152 in internal/pkg/deploy/pipeline_test.go?

oh I see sorry that's my bad! you can ignore the comment then, I thought they were in the same pkg :)

efekarakus · 2021-04-05T16:59:48Z

site/content/docs/manifest/pipeline.md

+The URI that identifies the Docker image to use for this build project. As of now, `aws/codebuild/amazonlinux2-x86_64-standard:3.0` is used by default.
+
+!!! info
+    Copilot does not support cross-account ECR repository or private registry other than ECR because [imagePullCredentialsType](https://docs.aws.amazon.com/codebuild/latest/APIReference/API_ProjectEnvironment.html#CodeBuild-Type-ProjectEnvironment-imagePullCredentialsType) is fixed to `CODEBUILD`.


What do you think of removing this information? I don't think any users asked to push images to a different registry 🤔 or different account

This is build image used for CodeBuild project, so I thought some users might want to specify their own image stored in another account's ECR repo or another private registry. However, it maybe edge case, so I'll remove this block as you suggested. I appreciate your nice feedback!

Thanks! let us know, once removed we can merge the PR :)

I just pushed it🚀

Lou1415926

Thank you so much for contributing!! This looks good to me!

Lou1415926 · 2021-04-06T16:10:45Z

internal/pkg/manifest/pipeline_test.go

@@ -261,6 +261,48 @@ stages:
 				},
 			},
 		},
+		"valid pipeline.yml with build": {


Thank you for writing all these tests! They are really helpful and comprehensive.

With aws#2125 we added a "build" field to pipeline, but forgot to update the integ tests that create the pipeline. This change adds the default build config so that the pipeline is created successfully.

Addresses aws#2106 * pipeline init behavior * generates manifest with build property automatically. * pipeline update behavior * if build property is not specified in pipeline manifest, default image is used. * if build property is specified explicitly in pipeline manifest, specified image is used. * Limitation: following images are descoped. * cross-account ECR repository * private registry other than ECR By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

tam0ri added 4 commits March 31, 2021 15:09

feat: specifying build image

19c5478

remove unnecessary code.

bacf01d

Add tests.

d85e76d

tam0ri requested a review from a team as a code owner April 1, 2021 15:46

tam0ri requested a review from iamhopaul123 April 1, 2021 15:46

amazon-ecs-cli-v2-pr-manager requested a review from Lou1415926 April 1, 2021 15:46

efekarakus reviewed Apr 2, 2021

View reviewed changes

Apply feedback.

c1f20e1

efekarakus approved these changes Apr 5, 2021

View reviewed changes

efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 5, 2021

Lou1415926 approved these changes Apr 6, 2021

View reviewed changes

remove info block

2dd9f4c

efekarakus approved these changes Apr 6, 2021

View reviewed changes

efekarakus removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 6, 2021

Merge branch 'mainline' into feat/buildimage-for-pipeline

443c8c8

efekarakus added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Apr 13, 2021

efekarakus and others added 2 commits April 14, 2021 14:36

chore: fix integ tests for pipelines

ac47e63

Merge branch 'mainline' into feat/buildimage-for-pipeline

54624ea

mergify bot merged commit 2a8458b into aws:mainline Apr 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(manifest): support codebuild environment image config #2125

feat(manifest): support codebuild environment image config #2125

tam0ri commented Apr 1, 2021

efekarakus left a comment

efekarakus Apr 1, 2021

efekarakus Apr 1, 2021

efekarakus Apr 1, 2021

efekarakus Apr 2, 2021

efekarakus Apr 2, 2021

efekarakus Apr 2, 2021

tam0ri commented Apr 2, 2021

efekarakus left a comment

efekarakus Apr 5, 2021

tam0ri Apr 6, 2021

efekarakus Apr 6, 2021

efekarakus Apr 5, 2021

tam0ri Apr 6, 2021

efekarakus Apr 6, 2021

tam0ri Apr 6, 2021

Lou1415926 left a comment

Lou1415926 Apr 6, 2021

	Build: build,
	Build: deploy.PipelineBuildFromManifest(pipeline.Build),

feat(manifest): support codebuild environment image config #2125

feat(manifest): support codebuild environment image config #2125

Conversation

tam0ri commented Apr 1, 2021

efekarakus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tam0ri commented Apr 2, 2021

efekarakus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lou1415926 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment