Avoid constant re-deployment of charts with multiple inline dependencies #1272
Conversation
:-) Sorry about that @ncabatoff, I was reluctant to remove the fruits of your hard work, but also keen to push through the simpler implementation. |
| hapi_release "k8s.io/helm/pkg/proto/hapi/release" | ||
|
|
||
| ifv1 "github.com/weaveworks/flux/apis/helm.integrations.flux.weave.works/v1alpha2" | ||
| "github.com/weaveworks/flux/git" | ||
| ifclientset "github.com/weaveworks/flux/integrations/client/clientset/versioned" | ||
| helmop "github.com/weaveworks/flux/integrations/helm" | ||
| "github.com/weaveworks/flux/integrations/helm/release" | ||
|
|
||
| "github.com/ncabatoff/go-seq/seq" |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
| return ret | ||
| } | ||
|
|
||
| type anySlice []*google_protobuf.Any |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
| sort.Sort(anySlice(nc.Files)) | ||
| sort.Sort(templateSlice(nc.Templates)) | ||
|
|
||
| nc.Metadata.Sources = sortStrings(nc.Metadata.Sources) |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
…slices in chart structures whose ordering is arbitrary, e.g. depended-upon charts.
ncabatoff
force-pushed
the
1235-charts-with-deps-continuously-upgraded
branch
from
4f74697 to
60e7717
Compare
|
What did you think about making the diff logging be opt-in via command-line option, for security reasons? |
Yes, good idea. How about:
|
| @@ -330,7 +333,11 @@ func (c *Controller) enqueueUpateJob(old, new interface{}) { | |||
|
|
|||
| if diff := cmp.Diff(oldFhr.Spec, newFhr.Spec); diff != "" { | |||
| c.logger.Log("info", "UPGRADING release") | |||
| c.logger.Log("info", fmt.Sprintf("Custom Resource driven release upgrade, diff:\n%s", diff)) | |||
| if c.logDiffs { | |||
| c.logger.Log("info", fmt.Sprintf("Custom Resource driven release upgrade, diff:\n%s", diff)) | |||
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Fix for #1235. Sort slices in chart structures whose ordering is arbitrary, e.g. depended-upon charts, so that the charts can be properly compared.
Right now it logs the diff between values and the diff between charts when there are discrepancies. If this PR is accepted we should probably add an option and make the logging behaviour opt-in: even though sensitive data should not be present in charts or values, it could be, and there's no reason to make a bad security situation worse.
The logging of diffs works great when processing a git-driven change. When reverting a manual change made to a release's values it doesn't work so well: we get the entire before and after config yaml, making for a very large and hard to read diff if there are a lot of values. It seems that the Helm API isn't populating Config.Values, only Config.Raw. I haven't found a way around this yet.
I wrote some tests that demonstrated the problem that passed with this fix, but sadly the tests I was extending in integrations/helm/releasesync/releasesync_test.go seem to be gone on the master branch. I'm not really sad, I'm happy with the new chartsync.go and the deleted tests don't make sense in the new context, but I'll have to see what I can do it in terms of automated tests for this area of the code.