feat(manifest): allow additional docker build overrides in manifest #1059

bvtujo · 2020-06-26T19:07:49Z

This PR adds the ability to specify docker compose style build directives in the manifest.

For example, customers can now specify dockerfile, context, and args underneath the build key in the manifest, or simply specify a dockerfile path at build.

Example 1

image:
  build: path/to/MyDockerfile

This manifest will build the dockerfile specified at build, using the directory path/to as the build context.

Example 2

image:
  build:
    dockerfile: path/to/MyDockerfile
    context: path/to/service/code
    args:
      ABC: 123
      arg2: value2

This example will build the dockerfile at path/to/MyDockerfile using the directory path/to/service/code as the build context and passing the args as --build-arg flags.

Fixes #822 and #1030

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

internal/pkg/cli/svc_deploy.go

iamhopaul123

I'm not sure if adding context field will help completely fix our problematic docker build. Currently if your pwd is not the root workspace then it is very likely svc deploy will fail because our dockerfile path is a relative one and ./subscribers/Dockerfile doesn't exist since . denotes different path. #1030

Have a use case for that. If the user has two services "frontend" and "backend". After they deploy those services, the frontend service has bug and they'd like to cd into the folder to run svc show/logs/status easier. However, they have to cd back to the workspace root in order to run svc deploy again, which might not make sense.

I think maybe we could probably completely get rid of the . and require users to put their dockerfile in the workspace. The build field will be like build: subscribers/myDockerfile. What do you think?

bvtujo · 2020-06-26T19:58:58Z

@iamhopaul123 My thought was that our current behavior is pretty bad especially for go. Consider the following use cases:

The golang project layout standard suggests that you should keep your dockerfiles in a separate build/package directory (build/package/Dockerfile_fe e.g.), not the root of the service code. Without a context, these builds will assume that the docker context directory is build/package. This contains no code files, though, and will fail.
Also using go best practices, any service whose main function lives at cmd/svcName/main.go will also fail to build under any cirumstances unless the dockerfile and context directory are at the workspace root. If the dockerfile is located at cmd/svcName/Dockerfile, the build context will be inferred to be cmd/svcName and the build will fail if the main() function imports any packages at pkg/* or internal/pkg/*.

This is a separate problem from svc deploy failing if it is not run from the workspace root.

iamhopaul123 · 2020-06-26T20:25:52Z

@iamhopaul123 My thought was that our current behavior is pretty bad especially for go. Consider the following use cases:

The golang project layout standard suggests that you should keep your dockerfiles in a separate build/package directory (build/package/Dockerfile_fe e.g.), not the root of the service code. Without a context, these builds will assume that the docker context directory is build/package. This contains no code files, though, and will fail.

Also using go best practices, any service whose main function lives at cmd/svcName/main.go will also fail to build under any cirumstances unless the dockerfile and context directory are at the workspace root. If the dockerfile is located at cmd/svcName/Dockerfile, the build context will be inferred to be cmd/svcName and the build will fail if the main() function imports any packages at pkg/* or internal/pkg/*`.

This is a separate problem from svc deploy failing if it is not run from the workspace root.

Oh then that makes sense to me. Thanks for explaining!

bvtujo · 2020-07-13T17:31:39Z

This PR now includes a fix for #1030. Dockerfile paths and contexts are interpreted from the workspace root (one level up from the copilot dir) instead of pwd. Contexts are now backwards compatible--if a manifest does not contain a context field, we call filepath.Dir() on the dockerfile path, and pass that value to docker build.

iamhopaul123

Looks awesome! Just two suggestions

internal/pkg/cli/svc_deploy.go

internal/pkg/docker/docker.go

internal/pkg/manifest/backend_svc.go

iamhopaul123

🚢

bvtujo · 2020-07-14T17:51:12Z

This PR should be updated to ensure
a) the pipeline buildspec takes advantage of new fields
b) that we use Docker Compose syntax for additional configuration in our "build" field. For example:

image:
  build:
    context: context/dir
    dockerfile: path/to/dockerfile

As described in https://docs.docker.com/compose/compose-file/#build

I've added the do-not-merge label until we can work through our next decision on this matter

* Convert CopilotDirPath to public function on ws for use in docker build calls in Service Deploy. * Generate mocks * Make context default to Dockerfile directory for backwards compatibility.

…ext into one call

…re shared

efekarakus

Only minor comments, also great tests!

efekarakus · 2020-07-29T00:44:54Z

internal/pkg/cli/svc_deploy.go

+	// Assemble other parameters for build input.
+	dockerBuildInput.URI = uri
+	dockerBuildInput.ImageTag = o.ImageTag


nit: what do you think of moving this to within getBuildArgs()?

Done. Respository now handles making the URI, and getBuildArgs adds the image tag.

efekarakus · 2020-07-29T00:52:36Z

internal/pkg/cli/svc_deploy_test.go

+				m.EXPECT().ReadServiceManifest("serviceA").Times(1).Return(mockManifest, nil)
+			},
+		},
+		"using simple buildstring (backwards compatible)": {


efekarakus · 2020-07-29T00:54:07Z

internal/pkg/docker/docker.go

-func (r Runner) Build(uri, path, imageTag string, additionalTags ...string) error {
-	dfDir := filepath.Dir(path)
+// BuildArguments holds the arguments we can pass in as flags from the manifest.
+type BuildArguments struct {


v nice comments!

internal/pkg/docker/docker_test.go

internal/pkg/manifest/backend_svc.go

internal/pkg/manifest/svc.go

efekarakus · 2020-07-29T01:12:28Z

internal/pkg/manifest/svc.go

-	Build *string `yaml:"build"` // Path to the Dockerfile.
+	Build BuildArgsOrString `yaml:"build"` // Path to the Dockerfile.
+}
+


Looks good! I am not sure about returning a docker.BuildArguments instead of a manifest.DockerBuildArgs from the manifest pkg, but I'm okay with keeping it as is.

internal/pkg/manifest/svc.go

Co-authored-by: Efe Karakus <[email protected]>

…Opts.getBuildArgs to return docker.BuildArguments

kohidave

THANK YOU.

kohidave · 2020-08-04T01:59:40Z

internal/pkg/cli/svc_deploy.go

 	if !ok {
-		return "", fmt.Errorf("service %s does not have a dockerfile path", o.Name)
+		return nil, fmt.Errorf("service %s does not have required method Build()", o.Name)


Maybe something our users could grok easier:

manifest for service %s does not have a `build` section

Or something like that?

This is a weird error because it specifically has to do with the interface conversion to the dfArgs interface and making sure that the unmarshaled yaml fits into a BackendService or LoadBalancedWebService struct with the right methods attached.

kohidave · 2020-08-04T02:03:07Z

internal/pkg/cli/svc_deploy.go

 	}
-	return mf.DockerfilePath(), nil
+	wsRoot := filepath.Dir(copilotDir)


Is this needed? I believe CopilotDirPath returns a directory.

Yes, because we want the workspace root, not the Copilot dir. filepath.Dir is equivalent to the shell dirname command.

kohidave · 2020-08-04T02:06:03Z

internal/pkg/docker/docker_test.go

+		"success with build args": {
+			path: mockPath,
+			args: map[string]string{
+				"GOPROXY": "direct",


kohidave · 2020-08-04T02:08:49Z

internal/pkg/manifest/lb_web_svc.go

+}
+
+//BuildArgs returns a docker.BuildArguments object given a ws root directory.
+func (s *LoadBalancedWebService) BuildArgs(wsRoot string) *DockerBuildArgs {


Can we add this method to Service struct, so we don't always have to redefine it for every pattern? Or is it not possible (no prob if it's not, just curioust)

I tried but it's weird, because the method needs access to the Image member of the LoadBalancedWebService or BackendService struct, but I don't know how to cast a particular member of an unmarshaled struct to an interface instead of just the whole struct

bvtujo added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 26, 2020

bvtujo requested a review from a team as a code owner June 26, 2020 19:07

bvtujo self-assigned this Jun 26, 2020

bvtujo requested a review from kohidave June 26, 2020 19:07

bvtujo commented Jun 26, 2020

View reviewed changes

internal/pkg/cli/svc_deploy.go Outdated Show resolved Hide resolved

iamhopaul123 reviewed Jun 26, 2020

View reviewed changes

efekarakus mentioned this pull request Jun 29, 2020

Use the Dockerfile rel path against Copilot dir instead of pwd #1030

Closed

efekarakus changed the title ~~feat(manifest): Add context field for docker build configurability~~ feat(manifest): add context field for docker build configurability Jun 29, 2020

kohidave requested a review from a team July 7, 2020 22:27

bvtujo force-pushed the mft-context branch from 89d1469 to 2cbb8be Compare July 11, 2020 00:37

iamhopaul123 reviewed Jul 13, 2020

View reviewed changes

internal/pkg/cli/svc_deploy.go Outdated Show resolved Hide resolved

internal/pkg/docker/docker.go Outdated Show resolved Hide resolved

internal/pkg/manifest/backend_svc.go Show resolved Hide resolved

bvtujo removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jul 13, 2020

bvtujo requested review from efekarakus and iamhopaul123 July 13, 2020 22:49

iamhopaul123 approved these changes Jul 13, 2020

View reviewed changes

bvtujo added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jul 14, 2020

efekarakus mentioned this pull request Jul 16, 2020

Allow specifying Docker build args in manifest #1158

Closed

bvtujo added 8 commits July 17, 2020 15:50

Add context field to mft

f740e81

update mft comments

add851a

Add comments to Context() and context to testdata

637dccd

fix unit test expectations for context

5ef0e0a

feat(workspace): add CopilotDirPath as public function

fb58231

* Convert CopilotDirPath to public function on ws for use in docker build calls in Service Deploy. * Generate mocks * Make context default to Dockerfile directory for backwards compatibility.

Add mocking for UnmarshalService and combine dockerfile path and cont…

ec1e5fd

…ext into one call

move Dockerfile() and Context() functions to svc package since they a…

0689c6a

…re shared

chore(manifest): add custom YAML unmarshaler for docker build args

c07791a

Fix buildspec to remove extra spaces

ef7feb5

efekarakus reviewed Jul 29, 2020

View reviewed changes

bvtujo and others added 18 commits July 30, 2020 12:55

Update repository package to take docker.BuildArguments

2139b32

Update docker test cases to not use imageName()

f9cb57e

Update interfaces to take docker.BuildArguments

a247546

implement new docker args signature

129beb8

go mod tidy

7b38a38

Implement new docker args in task run

f97d542

rename Build to BuildArgs in package manifest

6bb3a15

Fix test data to reflect prior default

79dc17e

add sorting logic to build-arg parser for test stability

1cb5473

Use guard clauses, not else ifs, in svc.go

f83de96

Co-authored-by: Efe Karakus <[email protected]>

Comment to indicate successful unmarshaling

39befaf

Co-authored-by: Efe Karakus <[email protected]>

Remove unnecessary else clause in svc.go

b9b78ff

whitespace fixes

5d6e755

fix unjoined context and dockerfile paths in guard clauses

4d5861f

Merge branch 'master' into mft-context

bcf19a8

remove test case to match master

951e132

Merge branch 'master' into mft-context

d0ea656

refactor svc package to return manifest.DockerBuildArgs and svcDeploy…

b500b6c

…Opts.getBuildArgs to return docker.BuildArguments

kohidave approved these changes Aug 4, 2020

View reviewed changes

bvtujo added 5 commits August 3, 2020 21:01

Edit comment in manifests

80eaa2a

Update testdata to reflect manifest comment

d6aaa98

Remove dependency on aws in svc deploy

5c35c8b

Merge branch 'master' into mft-context

6424529

Fix tests in repository to handle new Build() signature

b22b186

bvtujo removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Aug 4, 2020

mergify bot merged commit 5584131 into aws:master Aug 5, 2020

bvtujo changed the title ~~feat(manifest): allow docker-compose style docker build config in manifest~~ feat(manifest): allow additional docker build overrides in manifest Aug 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(manifest): allow additional docker build overrides in manifest #1059

feat(manifest): allow additional docker build overrides in manifest #1059

bvtujo commented Jun 26, 2020 •

edited

Loading

iamhopaul123 left a comment •

edited

Loading

bvtujo commented Jun 26, 2020 •

edited

Loading

iamhopaul123 commented Jun 26, 2020

bvtujo commented Jul 13, 2020

iamhopaul123 left a comment

iamhopaul123 left a comment

bvtujo commented Jul 14, 2020

efekarakus left a comment

efekarakus Jul 29, 2020

bvtujo Jul 31, 2020

efekarakus Jul 29, 2020

efekarakus Jul 29, 2020

efekarakus Jul 29, 2020

kohidave left a comment

kohidave Aug 4, 2020

bvtujo Aug 4, 2020

kohidave Aug 4, 2020

bvtujo Aug 4, 2020

kohidave Aug 4, 2020

kohidave Aug 4, 2020

bvtujo Aug 4, 2020

feat(manifest): allow additional docker build overrides in manifest #1059

feat(manifest): allow additional docker build overrides in manifest #1059

Conversation

bvtujo commented Jun 26, 2020 • edited Loading

Example 1

Example 2

iamhopaul123 left a comment • edited Loading

Choose a reason for hiding this comment

bvtujo commented Jun 26, 2020 • edited Loading

iamhopaul123 commented Jun 26, 2020

bvtujo commented Jul 13, 2020

iamhopaul123 left a comment

Choose a reason for hiding this comment

iamhopaul123 left a comment

Choose a reason for hiding this comment

bvtujo commented Jul 14, 2020

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

kohidave 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

bvtujo commented Jun 26, 2020 •

edited

Loading

iamhopaul123 left a comment •

edited

Loading

bvtujo commented Jun 26, 2020 •

edited

Loading