Remote helm charts should not be upgraded by default #3274

dahovey · 2019-11-20T18:19:03Z

Fixes #2245.

Description

Adds config to HelmRelease upgradeOnChange.

User facing changes

Remote helm charts will NOT be upgraded by default.

Before

Remote helm charts would always be upgraded on local file changes.

After

By default, remote helm charts will not be upgraded on local file changes. User will need to add upgradeOnChange: true to any remote charts that need to be upgraded on changes.

Next PRs.

n/a

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes unit tests
Mentions any output changes.
Adds documentation as needed: user docs, YAML reference, CLI reference.
Adds integration tests if needed.

Reviewer Notes

The code flow looks good.
Unit test added.
User facing changes look good.

Release Notes

- Remote helm charts are not upgraded by default. Added `upgradeOnChange` property to override.

googlebot · 2019-11-20T18:19:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

codecov · 2019-11-20T18:33:02Z

Codecov Report

Merging #3274 into master will increase coverage by 0.00%.
The diff coverage is 57.14%.

Impacted Files	Coverage Δ
pkg/skaffold/schema/latest/config.go	`100.00% <ø> (ø)`
pkg/skaffold/deploy/helm.go	`79.83% <57.14%> (-0.44%)`	⬇️
...affold/kubernetes/portforward/kubectl_forwarder.go	`63.41% <0.00%> (+2.43%)`	⬆️

dahovey · 2019-11-20T18:39:38Z

@googlebot I signed it!

pkg/skaffold/schema/latest/config.go

briandealwis · 2019-11-27T15:04:46Z

pkg/skaffold/deploy/helm.go

@@ -204,6 +204,9 @@ func (h *HelmDeployer) deployRelease(ctx context.Context, out io.Writer, r lates
 	if !isInstalled {
 		args = append(args, "install", "--name", releaseName)
 		args = append(args, h.Flags.Install...)
+	} else if !h.shouldUpgradeOnChange(r) {
+		logrus.Infof("Release %s already installed...\n", releaseName)


Do you think we should handle and log remote separately?

Sorry not sure exactly what you mean. I am open to anything just needed the main feature of not upgrading remote charts.

Do you want more logging to indicate why the chart wasn't upgraded?

Sorry, I missed this :-( Yes, more logging: since we're changing behaviour, it would be worth including why and what can be done.

briandealwis · 2019-11-27T16:30:17Z

@dahovey the CLA bot doesn't have a record for you. Could you please try again?

dahovey · 2019-11-27T16:47:28Z

@googlebot I signed it!

googlebot · 2019-11-27T16:47:31Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

dahovey · 2019-12-16T18:43:39Z

@briandealwis Could you clarify what you mean by 'handle' remote separately?

dgageot · 2020-01-15T07:46:25Z

@briandealwis could you please take another look?

briandealwis

A little more logging, otherwise LGTM.

briandealwis · 2020-01-15T16:14:08Z

pkg/skaffold/deploy/helm.go

@@ -204,6 +204,9 @@ func (h *HelmDeployer) deployRelease(ctx context.Context, out io.Writer, r lates
 	if !isInstalled {
 		args = append(args, "install", "--name", releaseName)
 		args = append(args, h.Flags.Install...)
+	} else if !h.shouldUpgradeOnChange(r) {
+		logrus.Infof("Release %s already installed...\n", releaseName)


Sorry, I missed this :-( Yes, more logging: since we're changing behaviour, it would be worth including why and what can be done.

dahovey · 2020-02-18T16:18:45Z

I think I need some help with this. On my first attempt with this feature the schema had already been updated, but now when I tried to run ./hack/new_version.sh after running tests I got all sorts of new errors that seem to be unrelated to the updates I made.

I did add more logging when upgradeOnChange is not set and based upon remote value.

briandealwis

Sorry @dahovey for the delay. If you merge with HEAD and resolve the conflict, your PR should be much smaller now.

pkg/skaffold/deploy/helm.go

pkg/skaffold/schema/latest/config.go

nkubala · 2020-03-16T18:44:11Z

@dahovey sorry this kinda fizzled out, that's on us. would you mind quickly rebasing and regenerating the schema docs so that they're updating the correct version? let me know if you need any help with that.

jtag05 · 2020-03-26T00:09:25Z

@dahovey is there anything I can do to assist to get this ready to merge?

dgageot · 2020-04-14T10:07:55Z

@dahovey this just needs a rebase and I'll merge it

jtag05 · 2020-04-18T13:32:32Z

@dgageot would it be possible for someone else to pick this up? I'd be happy to refork if @dahovey isn't available to push this across the finish line.

…o dahovey/upgradeOnChange

dahovey · 2020-04-21T20:13:05Z

Hi all, I apologize for the delay.

I got a whole lot of failures running make test, but I've merged current changes and refactored based on helm.go changes

nkubala · 2020-04-22T19:03:13Z

@dahovey sorry to hear you ran into issues with the tests. I see a few things looking at the failures in travis:

since this fell a bit behind, we released a new config version since you sent this PR. you'll need to move your config changes to the latest config file (v2beta3)
looks like a compile error in pkg/skaffold/deploy/helm_test.go (the unit test)
the schemas are out of date because your changes fell behind the current schema. once you make your changes, run make schemas to generate the new ones and commit that

we run make test directly in CI, so running this locally you should be able to catch all these errors. let me know if you're having other issues with running this directly and I'll try and help.

tstromberg · 2020-04-30T17:41:29Z

@dahovey - need any help getting this PR pushed through?

…o dahovey/upgradeOnChange

dahovey · 2020-05-08T18:52:04Z

@tstromberg @nkubala Could one of you please help resolve this error? It is received when running make or make install.

E0508 11:50:00.044868   20910 library.go:108] Failed to find license for github.com/docker/go-metrics: no file/directory matching regexp "^(LICEN(S|C)E|COPYING|README|NOTICE)(\\.(txt|md))?$" found for /Users/davidhovey/go/src/github.com/dahovey/skaffold/vendor/github.com/docker/go-metrics
Error: one or more libraries have an incompatible/unknown license: map["unknown":["github.com/docker/go-metrics"]]

dahovey · 2020-05-08T21:27:13Z

I bumped schema version to v2beta4. I believe I am very close. Right now it appears checks failed, but the deploy test is passing.

Before the tests were refactored I believe I had it setup correctly to test that upgrade or install were not called. I couldn't figure out how to test whether a command was not called. Let me know any other changes needed.

Thanks for your patience.

nkubala · 2020-05-21T19:11:26Z

@dahovey that error you were getting comes from the go-licenses tool we use to calculate our licenses, which we really don't need to be doing on local builds....you can try doing a which licenses or which go-licenses and if you have any binaries delete them, then let skaffold install, otherwise if you can't make it work you can remove the generate-statik target from your Makefile. sorry this is so painful.

unfortunately this keeps falling behind on our release cadence so the schema version needs to keep getting bumped, but I'll do everything I can to make sure this gets merged in the next week and a half before our next release. would you mind doing one last rebase?

briandealwis · 2020-05-27T18:24:21Z

We just merged the version bump to v2beta5.

…o HEAD

… into HEAD

dahovey · 2020-05-27T19:16:24Z

@briandealwis @nkubala Thanks for your patience. This has been a learning experience. Hope everything looks better.

I still couldn't resolve the licensing error. I ran which for go-licenses and licenses but no results. I had to remove the generate-statik target

briandealwis · 2020-05-29T16:13:01Z

Thanks @dahovey!

nkubala · 2020-05-29T18:34:35Z

yes! awesome to see this one finally go in! thanks @dahovey for all your patience here, I think we've also learned a thing or two about staying on top of our reviews to make sure things don't fall behind because of us. hopefully your next contribution is a lot less painful 😁

Added config to HelmRelease

e7593b1

dahovey requested review from balopat, dgageot, nkubala and tejal29 as code owners November 20, 2019 18:19

pull-request-size bot added the size/M label Nov 20, 2019

googlebot added the cla: no label Nov 20, 2019

briandealwis reviewed Nov 27, 2019

View reviewed changes

googlebot added cla: yes and removed cla: no labels Nov 27, 2019

Fixed spelling error in comment for HelmRelease.upgradeOnChange

05b2a71

dgageot assigned briandealwis Jan 15, 2020

briandealwis reviewed Jan 15, 2020

View reviewed changes

dahovey added 3 commits February 17, 2020 10:41

Merge branch 'master' into dahovey/upgradeOnChange

dd8d6f9

Added more logging for 'upgradeOnChange' default values

fdbf65b

Upgrading schema to v2alpha4

c70e124

dahovey requested review from chanseokoh, loosebazooka and TadCordle as code owners February 17, 2020 21:17

briandealwis reviewed Feb 19, 2020

View reviewed changes

pkg/skaffold/deploy/helm.go Outdated Show resolved Hide resolved

pkg/skaffold/schema/latest/config.go Outdated Show resolved Hide resolved

Merge branch 'master' into dahovey/upgradeOnChange

4ad5dfa

Merge branch 'master' of github.com:GoogleContainerTools/skaffold int…

b576b75

…o dahovey/upgradeOnChange

Merge branch 'master' of github.com:GoogleContainerTools/skaffold int…

2ee194f

…o dahovey/upgradeOnChange

dahovey added 2 commits May 8, 2020 12:27

Fix for helm_test not building

6ff6b6e

bumped schema version to v2beta4

d43417a

dahovey added 4 commits May 27, 2020 11:33

Merge branch 'master' of github.com:GoogleContainerTools/skaffold int…

e91323f

…o HEAD

updated schema

7dc4557

Merge branch 'dahovey/upgradeOnChange' of github.com:dahovey/skaffold…

8285de2

… into HEAD

Adding merged changes

a659e91

dahovey requested a review from tstromberg as a code owner May 27, 2020 18:48

briandealwis added the kokoro:run runs the kokoro jobs on a PR label May 29, 2020

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 29, 2020

briandealwis merged commit 6b744d5 into GoogleContainerTools:master May 29, 2020

briandealwis changed the title ~~Added config to HelmRelease~~ Remote helm charts should not be upgraded by default May 29, 2020

briandealwis mentioned this pull request May 29, 2020

Helm release errors out on filesystem write because of labeling issue #4277

Closed

fmotrifork mentioned this pull request Jun 12, 2020

skaffold 1.11.0 fishworks/fish-food#783

Merged

nkubala mentioned this pull request Jul 6, 2020

Remote helm charts should be upgraded by default #4431

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote helm charts should not be upgraded by default #3274

Remote helm charts should not be upgraded by default #3274

dahovey commented Nov 20, 2019 •

edited by briandealwis

Loading

googlebot commented Nov 20, 2019

codecov bot commented Nov 20, 2019 •

edited

Loading

dahovey commented Nov 20, 2019

briandealwis Nov 27, 2019

dahovey Nov 27, 2019

briandealwis Jan 15, 2020

briandealwis commented Nov 27, 2019

dahovey commented Nov 27, 2019

googlebot commented Nov 27, 2019

dahovey commented Dec 16, 2019

dgageot commented Jan 15, 2020

briandealwis left a comment

briandealwis Jan 15, 2020

dahovey commented Feb 18, 2020

briandealwis left a comment

nkubala commented Mar 16, 2020

jtag05 commented Mar 26, 2020

dgageot commented Apr 14, 2020

jtag05 commented Apr 18, 2020

dahovey commented Apr 21, 2020 •

edited

Loading

nkubala commented Apr 22, 2020

tstromberg commented Apr 30, 2020

dahovey commented May 8, 2020

dahovey commented May 8, 2020

nkubala commented May 21, 2020

briandealwis commented May 27, 2020

dahovey commented May 27, 2020

briandealwis commented May 29, 2020

nkubala commented May 29, 2020

Remote helm charts should not be upgraded by default #3274

Remote helm charts should not be upgraded by default #3274

Conversation

dahovey commented Nov 20, 2019 • edited by briandealwis Loading

googlebot commented Nov 20, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

codecov bot commented Nov 20, 2019 • edited Loading

Codecov Report

dahovey commented Nov 20, 2019

briandealwis Nov 27, 2019

Choose a reason for hiding this comment

dahovey Nov 27, 2019

Choose a reason for hiding this comment

briandealwis Jan 15, 2020

Choose a reason for hiding this comment

briandealwis commented Nov 27, 2019

dahovey commented Nov 27, 2019

googlebot commented Nov 27, 2019

dahovey commented Dec 16, 2019

dgageot commented Jan 15, 2020

briandealwis left a comment

Choose a reason for hiding this comment

briandealwis Jan 15, 2020

Choose a reason for hiding this comment

dahovey commented Feb 18, 2020

briandealwis left a comment

Choose a reason for hiding this comment

nkubala commented Mar 16, 2020

jtag05 commented Mar 26, 2020

dgageot commented Apr 14, 2020

jtag05 commented Apr 18, 2020

dahovey commented Apr 21, 2020 • edited Loading

nkubala commented Apr 22, 2020

tstromberg commented Apr 30, 2020

dahovey commented May 8, 2020

dahovey commented May 8, 2020

nkubala commented May 21, 2020

briandealwis commented May 27, 2020

dahovey commented May 27, 2020

briandealwis commented May 29, 2020

nkubala commented May 29, 2020

dahovey commented Nov 20, 2019 •

edited by briandealwis

Loading

codecov bot commented Nov 20, 2019 •

edited

Loading

dahovey commented Apr 21, 2020 •

edited

Loading