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

Support explicitly specifying per-artifact language runtimes #3943

Closed

Conversation

briandealwis
Copy link
Member

Fixes: #2350
Merge after: #3942 (introduces v2beta2)

Description

This PR adds a new runtimes attribute to build artifacts to allow a user to explicitly specify the set of language runtimes used in that artifact. This ability will be used by skaffold debug and helps with Go debugging, where debug's heuristics depend on defining some set of Go environment variables (e.g., GOGC, GODEBUG, GOTRACEBACK). It also helps with Buildpacks as there is no way for users to cause a new environment variables to be added to the runtime image.

For example, examples/getting-started/skaffold.yaml will have the following change which will allow us to setup an Alpine-compatible Delve debugger.

 build:
   artifacts:
   - image: skaffold-example
+    runtimes: [go, musl]

Skaffold's deployers do not have any access to the original configuration model (as defined by the skaffold.yaml). To bridge this disconnect, I've added a new field to pkg/skaffold/build's Artifact to include the corresponding configuration artifact definition:

 type Artifact struct {
 	ImageName string `json:"imageName"`
 	Tag       string `json:"tag"`
+	// Config is the  corresponding artifact object from `skaffold.yaml`.
+	Config latest.Artifact `json:"-"`
  }

User facing changes (remove if N/A)

Adds new runtimes field.

Follow-up Work (remove if N/A)

I need to create a MUSL-based Delve container (GoogleContainerTools/container-debug-support#30) and then add support to transform_go to use the appropriate image depending on the musl or glibc runtime.

@codecov
Copy link

codecov bot commented Apr 11, 2020

Codecov Report

Merging #3943 into master will decrease coverage by 0.06%.
The diff coverage is 48.14%.

Impacted Files Coverage Δ
pkg/skaffold/build/build.go 0.00% <ø> (ø)
pkg/skaffold/debug/debug.go 44.44% <0.00%> (-0.72%) ⬇️
pkg/skaffold/debug/transform_go.go 84.37% <0.00%> (-1.80%) ⬇️
pkg/skaffold/debug/transform_jvm.go 92.78% <0.00%> (-1.96%) ⬇️
pkg/skaffold/debug/transform_nodejs.go 76.52% <0.00%> (-1.36%) ⬇️
pkg/skaffold/debug/transform_python.go 82.75% <0.00%> (-1.95%) ⬇️
pkg/skaffold/schema/latest/config.go 100.00% <ø> (ø)
pkg/skaffold/schema/v2alpha4/upgrade.go 77.77% <ø> (ø)
pkg/skaffold/schema/versions.go 75.00% <ø> (ø)
pkg/skaffold/debug/transform.go 87.87% <40.00%> (-1.50%) ⬇️
... and 4 more

@dgageot
Copy link
Contributor

dgageot commented Apr 14, 2020

@briandealwis Could you please rebase?

@@ -28,6 +28,8 @@ import (
type Artifact struct {
ImageName string `json:"imageName"`
Tag string `json:"tag"`
// Config is the corresponding artifact object from `skaffold.yaml`.
Config latest.Artifact `json:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand where this gets populated from?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrmm that seems to have somehow disappeared. And clearly a test is needed.

@dgageot dgageot self-assigned this Apr 14, 2020
@dgageot dgageot marked this pull request as draft April 21, 2020 08:24
@dgageot dgageot removed the !! wip !! label Apr 21, 2020
@dgageot dgageot removed their assignment Jun 5, 2020
@nkubala
Copy link
Contributor

nkubala commented Sep 10, 2020

@briandealwis what's the latest on this?

@briandealwis
Copy link
Member Author

It needs a substantial update. But it should be much easier with the new configs.

@nkubala
Copy link
Contributor

nkubala commented Sep 17, 2020

closing this for now, please reopen when its ready

@nkubala nkubala closed this Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants