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

Build debuggable containers #4606

Merged
merged 11 commits into from
Aug 23, 2020

Conversation

gsquared94
Copy link
Contributor

@gsquared94 gsquared94 commented Jul 31, 2020

Fixes #4370
Add default build args for docker and buildpacks when building for Debug vs non Debug. Also handle skaffold image cache updates.

Redo #4467. No need to extract out ImageOptions since #4598 introduces a Config interface anyways.

User facing changes (remove if N/A)
Dockerfile can expose ARG SKAFFOLD_GO_GCFLAGS and expect Skaffold to supply meaningful values.
Buildpacks auto set GOOGLE_GCFLAGS build options.
Additionally the run mode dev, debug, etc. is exposed as SKAFFOLD_RUN_MODE

@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #4606 into master will increase coverage by 0.17%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4606      +/-   ##
==========================================
+ Coverage   73.20%   73.38%   +0.17%     
==========================================
  Files         339      341       +2     
  Lines       13320    13505     +185     
==========================================
+ Hits         9751     9910     +159     
- Misses       2958     2976      +18     
- Partials      611      619       +8     
Impacted Files Coverage Δ
pkg/skaffold/build/gcb/docker.go 81.81% <50.00%> (-4.85%) ⬇️
pkg/skaffold/build/local/local.go 63.63% <50.00%> (ø)
pkg/skaffold/docker/image.go 73.36% <50.00%> (-2.64%) ⬇️
pkg/skaffold/docker/build_args.go 70.37% <70.37%> (ø)
pkg/skaffold/build/local/docker.go 78.18% <71.42%> (-2.59%) ⬇️
pkg/skaffold/build/cache/hash.go 73.23% <84.61%> (-2.71%) ⬇️
pkg/skaffold/docker/parse.go 89.94% <84.61%> (-0.43%) ⬇️
pkg/skaffold/build/buildpacks/env.go 89.47% <89.47%> (ø)
pkg/skaffold/build/buildpacks/default_args.go 100.00% <100.00%> (ø)
pkg/skaffold/build/buildpacks/lifecycle.go 81.57% <100.00%> (-1.14%) ⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a5c603...f7a70cc. Read the comment docs.

Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

LGTM!

pkg/skaffold/build/cache/hash_test.go Show resolved Hide resolved
@tejal29
Copy link
Member

tejal29 commented Aug 11, 2020

Dockerfile can expose ARG SKAFFOLD_GO_GCFLAGS and ARG SKAFFOLD_GO_LDFLAGS and expect Skaffold to supply meaningful values.

Why is this change required? Setting "GO_GC_FLAGS" and "GO_LDFLAGS" in dockerfile is a common practice. Changing this to "SKAFFOLD_GO_GCFLAGS" might break existing user's dockerfile

@gsquared94 gsquared94 closed this Aug 11, 2020
@gsquared94
Copy link
Contributor Author

Dockerfile can expose ARG SKAFFOLD_GO_GCFLAGS and ARG SKAFFOLD_GO_LDFLAGS and expect Skaffold to supply meaningful values.

Why is this change required? Setting "GO_GC_FLAGS" and "GO_LDFLAGS" in dockerfile is a common practice. Changing this to "SKAFFOLD_GO_GCFLAGS" might break existing user's dockerfile

I'm not very opinionated about this. @briandealwis had suggested prefixing SKAFFOLD_, I think as a way to distinguish that it's an environment variable that we pass in explicitly. The user may set regular GO_LDFLAGS and GO_GCFLAGS in his Dockerfile to any arbitrary value and ignore skaffold supplied values entirely. I'm not sure why you think that it would break them though.

@gsquared94
Copy link
Contributor Author

@briandealwis @tejal29 @nkubala PTAL.

pkg/skaffold/build/cache/hash.go Show resolved Hide resolved
pkg/skaffold/build/buildpacks/default_args.go Outdated Show resolved Hide resolved
pkg/skaffold/docker/default_args.go Outdated Show resolved Hide resolved
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

One question and a few optional changes

pkg/skaffold/build/buildpacks/default_args.go Outdated Show resolved Hide resolved
pkg/skaffold/build/buildpacks/lifecycle.go Outdated Show resolved Hide resolved
pkg/skaffold/docker/parse.go Outdated Show resolved Hide resolved
pkg/skaffold/docker/parse.go Outdated Show resolved Hide resolved
pkg/skaffold/util/util.go Outdated Show resolved Hide resolved
@gsquared94 gsquared94 merged commit 4652950 into GoogleContainerTools:master Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skaffold helps build debuggable containers
4 participants