-
Notifications
You must be signed in to change notification settings - Fork 193
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
Conversation
Signed-off-by: Raffael Sahli <[email protected]>
@@ -430,8 +435,6 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context, | |||
|
|||
readyMessage = fmt.Sprintf("Fetched and packaged revision: %s", newArtifact.Revision) | |||
readyReason = sourcev1.ChartPackageSucceededReason |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 <[email protected]>
There was a problem hiding this 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!
Regarding your note about |
Creating a HelmRelease/Chart with a
valuesFile
declaration ends with a 0bytes tgz on the storage path.Reproducing manifests:
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:
Copy()
method in the storage package is quite the same asAtomicWriteFile()
, only difference is the chmod, might merge these two as in either case the mode can be set I reckon.