fix: Deploy Application with Helm chart pointed by digest #25079
Conversation
❗ Preview Environment deployment failed on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
pgodowski
force-pushed
the
fix/helm-charts-oci-digests
branch
from
ea7e0fa to
7198788
Compare
Signed-off-by: Piotr Godowski <[email protected]>
pgodowski
force-pushed
the
fix/helm-charts-oci-digests
branch
from
a8fc622 to
90c7b21
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #25079 +/- ##
=========================================
Coverage ? 62.22%
=========================================
Files ? 352
Lines ? 49229
Branches ? 0
=========================================
Hits ? 30633
Misses ? 15662
Partials ? 2934 ☔ View full report in Codecov by Sentry. |
util/helm/cmd.go
Outdated
| func (c *Cmd) Fetch(repo, chartName, version, destination string, creds Creds, passCredentials bool) (string, error) { | ||
| args := []string{"pull", "--destination", destination} | ||
| if version != "" { | ||
| if version != "" && !strings.HasPrefix(version, "sha256:") { |
There was a problem hiding this comment.
Will be easier to read formatted as:
if version != "" {
if strings.HasPrefix(version, "sha256:") {
// ...
} else {
// ...
}
}There was a problem hiding this comment.
I added versions.IsDigest function to make the code look cleaner.
| // constraint. | ||
| // If the revision is a constraint, but no tag satisfies that constraint, then it returns an error. | ||
| func MaxVersion(revision string, tags []string) (string, error) { | ||
| // Check if the revision is a SHA256 digest (used in OCI repositories) |
There was a problem hiding this comment.
For these general purpose functions, is there any other use for digests except for this helm use-case? If not, I would argue the fix does not belong to util/versions/tags.go.
There was a problem hiding this comment.
created versions.IsDigest in util/versions/digest.go
util/helm/cmd.go
Outdated
| args := []string{"pull", "--destination", destination} | ||
| if version != "" { | ||
| if version != "" && !strings.HasPrefix(version, "sha256:") { | ||
| // it is ok to use version flag |
There was a problem hiding this comment.
please write detailed comment on the changes behind it. Why is it okay to use the version flag? You can possibly link the issue as well so any dev who reads this code understand why this was set.
There was a problem hiding this comment.
added comments and refactored the code introducing versions.IsDigest function call
util/helm/cmd.go
Outdated
| if version != "" && !strings.HasPrefix(version, "sha256:") { | ||
| // it is ok to use version flag | ||
| args = append(args, "--version", version) | ||
| } else if version != "" && strings.HasPrefix(version, "sha256:") { | ||
| // For sha256 digest, append it to the chart name | ||
| chartName = fmt.Sprintf("%s@%s", chartName, version) | ||
| } | ||
|
|
There was a problem hiding this comment.
| if version != "" && !strings.HasPrefix(version, "sha256:") { | |
| // it is ok to use version flag | |
| args = append(args, "--version", version) | |
| } else if version != "" && strings.HasPrefix(version, "sha256:") { | |
| // For sha256 digest, append it to the chart name | |
| chartName = fmt.Sprintf("%s@%s", chartName, version) | |
| } | |
| if version != "" { | |
| if strings.HasPrefix(version, "sha256:") { | |
| chartName = fmt.Sprintf("%s@%s", chartName, version) | |
| } else { | |
| args = append(args, "--version", version) | |
| } | |
| } |
Please write comments too.
There was a problem hiding this comment.
refactored with introduction of IsDigest function.
There was a problem hiding this comment.
The code looks good to me. Tagging @blakepettersson for reviews!
|
It might make sense to have this with the Helm client as well, but I'd like to hold off to see what Helm 4 is going to do in terms of OCI support. |
|
Thank you all for the comments - will respond within the next 2 days. |
Signed-off-by: Piotr Godowski <[email protected]>
pgodowski
force-pushed
the
fix/helm-charts-oci-digests
branch
from
b0d1879 to
18b896a
Compare
We would like to make it working with Helm client, due to the already existing investement in Helm based repos and relative low energy required to make Argo working fine with Helm/OCI/digests.
Based on my study of Helm4 alpha content, it will further evolve OCI repositories support in slightly different aspects, i.e. to be able to discover Helm charts from OCI repositories (which does not exist with Helm3 - there is no helm chart repos/ |
4 participants
Checklist:
Fixes #25078
In my organisation, we install Helm charts packaged into OCI repository, by pointing them via digest, e.g.
However, prior to the fix, one couldn't create
Applicationwith Helm OCI repository, whentargetRevisionis sha256 digest value, and not the image tag.The underlying problem is with the Helm CLI commands produced by Argo, when image digest is being used, i.e.: the usage of
--versionflag, which Helm CLI tries to parse as a semver version, which obviously would fail when sha256 digest value is provided.The Helm community discussed options what should be the proper helm CLI arguments, when image digests are used:
With the conclusion, that whenever digest is used, the flag
--versionmust not be used, and image digest must be specified as part of the helm chart name reference.The fix is implemented in such a way that it detects whether
targetRevisionis either digest (sha256:prefix) or not and used different variant of Helm CLI flags depending on the actual condition.Evidence from the ArgoUI that now both Helm charts pointed by semver or by digest work just fine (before the fix, only semver in targetRevision worked fine).
summary of
make test: