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

Fix chart with custom valuesFile (0bytes tgz) #286

Merged
merged 2 commits into from
Feb 9, 2021

Conversation

raffis
Copy link
Contributor

@raffis raffis commented Feb 9, 2021

Creating a HelmRelease/Chart with a valuesFile declaration ends with a 0bytes tgz on the storage path.

$ ls -l /tmp/storage/helmchart/flux-system/podinfo
total 0
-rw-r--r--  1 raffis  wheel   0 Feb  9 09:48 podinfo-5.1.4.tgz
-rw-r--r--  1 raffis  wheel   0 Feb  9 09:48 podinfo-5.1.4.tgz.lock
lrwxr-xr-x  1 raffis  wheel  60 Feb  9 09:48 podinfo-latest.tgz -> /tmp/storage/helmchart/flux-system/podinfo/podinfo-5.1.4.tgz

Reproducing manifests:

apiVersion: source.toolkit.fluxcd.io/v1beta1
kind: HelmRepository
metadata:
  name: podinfo
  namespace: flux-system
spec:
  interval: 5m
  url: https://stefanprodan.github.io/podinfo
---
apiVersion: source.toolkit.fluxcd.io/v1beta1
kind: HelmChart
metadata:
  name: podinfo
  namespace: flux-system
spec:
  interval: 1m
  sourceRef:
    kind: HelmRepository
    name: podinfo
  chart: podinfo
  valuesFile: values-prod.yaml

The problem is that the it falls through to default as well and executes the atomicwrite as well.
This pr fixes that behavior.

Two other hints:

  • Might need to add a test to validate the written tgz
  • The Copy() method in the storage package is quite the same as AtomicWriteFile(), only difference is the chmod, might merge these two as in either case the mode can be set I reckon.

Signed-off-by: Raffael Sahli <raffael.sahli@doodle.com>
@hiddeco hiddeco added area/helm Helm related issues and pull requests bug Something isn't working labels Feb 9, 2021
@hiddeco hiddeco requested a review from relu February 9, 2021 09:28
@@ -430,8 +435,6 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,

readyMessage = fmt.Sprintf("Fetched and packaged revision: %s", newArtifact.Revision)
readyReason = sourcev1.ChartPackageSucceededReason
Copy link
Member

Choose a reason for hiding this comment

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

I think just adding a break statement here would solve the problem. The idea of the default block fallthrough was to prevent duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah thats fine as well, I've changed it to just having a break here.

@relu
Copy link
Member

relu commented Feb 9, 2021

Nice catch, @raffis! 🎣

I can definitely see the problem. I added a comment about the proposed solution with the hope that we can prevent the introduced duplication of the package writing logic block.

Signed-off-by: Raffael Sahli <raffael.sahli@doodle.com>
Copy link
Member

@relu relu left a comment

Choose a reason for hiding this comment

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

LGTM ✔️

Thank you, @raffis!

@relu
Copy link
Member

relu commented Feb 9, 2021

Regarding your note about Copy() and AtomicWriteFile(), I think it does make sense to extract the common part or merge the two. Feel free to submit a PR if you want to work on that, it would be much appreciated.

@relu relu merged commit b501172 into fluxcd:main Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants