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

Make IMAGE_REPO and IMAGE_TAG templated values in custom build and helm deploy #4278

Merged
merged 9 commits into from
Jun 3, 2020

Conversation

gsquared94
Copy link
Contributor

@gsquared94 gsquared94 commented May 29, 2020

Make IMAGE_REPO and IMAGE_TAG templated values in custom build and helm deploy

Fixes: #3343
Description
User facing changes (remove if N/A)
From newly added example project templated-fields:

IMAGE_REPO and IMAGE_TAG are available as templated fields in build.sh file

#from build.sh, line 13
img="${IMAGE_REPO}:${IMAGE_TAG}"

and also in the helm deploy section of the skaffold config, which configures artifact skaffold-templated to build with build.sh:

// from skaffold.yaml, line 22-24
      setValueTemplates:
          imageRepo: "{{.IMAGE_REPO}}"
          imageTag: "{{.IMAGE_TAG}}"

@codecov
Copy link

codecov bot commented May 29, 2020

Codecov Report

Merging #4278 into master will increase coverage by 0.01%.
The diff coverage is 59.25%.

Impacted Files Coverage Δ
pkg/skaffold/deploy/helm.go 78.87% <59.09%> (-0.97%) ⬇️
pkg/skaffold/build/custom/script.go 53.19% <60.00%> (+0.81%) ⬆️
pkg/skaffold/deploy/kubectl/visitor.go 95.83% <0.00%> (+2.28%) ⬆️
...affold/kubernetes/portforward/kubectl_forwarder.go 63.41% <0.00%> (+2.43%) ⬆️

@gsquared94 gsquared94 added the kokoro:run runs the kokoro jobs on a PR label May 29, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 29, 2020
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.

I took a quick first pass, but I'm not a Helm expert.

examples/templated-fields/README.md Outdated Show resolved Hide resolved
examples/templated-fields/README.md Outdated Show resolved Hide resolved
examples/templated-fields/go.mod Outdated Show resolved Hide resolved
examples/templated-fields/main.go Outdated Show resolved Hide resolved
examples/templated-fields/README.md Outdated Show resolved Hide resolved
@gsquared94 gsquared94 requested a review from briandealwis May 29, 2020 21:53
@gsquared94 gsquared94 added the kokoro:run runs the kokoro jobs on a PR label May 29, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 29, 2020
@@ -554,18 +536,34 @@ func installArgs(r latest.HelmRelease, builds []build.Artifact, valuesSet map[st
args = append(args, "--set-string", value)
}

args, err = constructOverrideArgs(&r, builds, args, func(k string) {
Copy link
Contributor Author

@gsquared94 gsquared94 Jun 1, 2020

Choose a reason for hiding this comment

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

@tstromberg currently skaffold render for helm passes empty values for image.name and image.tag. Also it skips over --set-file. Shouldn't deploy and render lead to identical kubernetes yaml files? That's what I assumed and extracted the common constructOverrideArgs method. Let me know if that shouldn't be the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems logical to me that deploy and render should yield identical manifests, but there are likely corner cases to be considered. @nkubala - thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

there are some upcoming features to render that will shift the paradigms a bit from the conventional deploy flow, but for the most part they should be going through the same code paths so this is fine.

@gsquared94 gsquared94 requested a review from tstromberg June 1, 2020 21:46
Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

Code is LGTM. I'll defer to @nkubala for alignment with Skaffold's principles.

integration/examples/templated-fields/skaffold.yaml Outdated Show resolved Hide resolved
@@ -554,18 +536,34 @@ func installArgs(r latest.HelmRelease, builds []build.Artifact, valuesSet map[st
args = append(args, "--set-string", value)
}

args, err = constructOverrideArgs(&r, builds, args, func(k string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems logical to me that deploy and render should yield identical manifests, but there are likely corner cases to be considered. @nkubala - thoughts?

@@ -554,18 +536,34 @@ func installArgs(r latest.HelmRelease, builds []build.Artifact, valuesSet map[st
args = append(args, "--set-string", value)
}

args, err = constructOverrideArgs(&r, builds, args, func(k string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

there are some upcoming features to render that will shift the paradigms a bit from the conventional deploy flow, but for the most part they should be going through the same code paths so this is fine.

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.

Proposal: Cleanup / Standardize value template / env vars around IMAGE
6 participants