feat: Support oci-based helm repository #4018

haoshuwei · 2020-07-29T05:43:53Z

features for proposal #3349
Signed-off-by: haoshuwei [email protected]

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
The title of the PR states what changed and the related issues number (used for the release note).
I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
Optional. My organization is added to USERS.md.
I've signed the CLA and my build is green (troubleshooting builds).

codecov-commenter · 2020-08-03T06:55:56Z

Codecov Report

Merging #4018 into master will decrease coverage by 0.24%.
The diff coverage is 18.35%.

@@            Coverage Diff             @@
##           master    #4018      +/-   ##
==========================================
- Coverage   42.89%   42.65%   -0.25%     
==========================================
  Files         124      124              
  Lines       18255    18386     +131     
==========================================
+ Hits         7831     7842      +11     
- Misses       9423     9534     +111     
- Partials     1001     1010       +9

Impacted Files	Coverage Δ
cmd/argocd/commands/app.go	`5.50% <0.00%> (-0.02%)`	⬇️
cmd/argocd/commands/repo.go	`0.00% <0.00%> (ø)`
pkg/apis/application/v1alpha1/types.go	`58.73% <0.00%> (-0.37%)`	⬇️
util/settings/settings.go	`42.26% <ø> (ø)`
util/helm/cmd.go	`28.12% <10.38%> (-13.94%)`	⬇️
util/argo/argo.go	`73.12% <25.00%> (-1.44%)`	⬇️
util/helm/client.go	`48.48% <28.26%> (-10.58%)`	⬇️
reposerver/repository/repository.go	`59.90% <50.00%> (ø)`
server/application/application.go	`29.47% <100.00%> (ø)`
util/db/repository.go	`65.78% <100.00%> (+0.22%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 997f38d...593a9e0. Read the comment docs.

haoshuwei · 2020-08-03T10:03:50Z

Hi @jannfis，can you help have a review about this PR, proposal: Support to use Helm 3 with OCI for package distribution

xianlubird · 2020-08-04T02:26:43Z

@jessesuen Pls take a look.

jannfis

Hey @haoshuwei, thanks a lot for your PR - I have made a first review, please find my comments below.

cmd/argocd/commands/repo.go

util/argo/argo.go

util/helm/client.go

util/helm/cmd.go

cmd/argocd/commands/repo.go

pkg/apis/application/v1alpha1/types.go

haoshuwei · 2020-08-11T13:22:28Z

@alexmt @jannfis I have updated the PR to use --enable-oci instead of a new type helm-oci. Please help have a refresh review, thanks.

Signed-off-by: haoshuwei <[email protected]>

haoshuwei · 2020-08-25T02:40:56Z

@alexmt can you please help have a look at this PR?

SayakMukhopadhyay · 2020-09-01T11:48:57Z

@alexmt @jannfis I have updated the PR to use --enable-oci instead of a new type helm-oci. Please help have a refresh review, thanks.

Would enabling the feature still support older helm repos? If it does, does the controller figure out if the target repo is a OCI repo or not?

wtam2018 · 2020-09-09T05:08:59Z

pkg/apiclient/repository/repository.pb.go

-	Name                 string   `protobuf:"bytes,10,opt,name=name,proto3" json:"name,omitempty"`
+	Name string `protobuf:"bytes,10,opt,name=name,proto3" json:"name,omitempty"`
+	// Whether helm-oci support should be enabled for this repo
+	EnableOci            bool     `protobuf:"varint,11,opt,name=enableOci,proto3" json:"enableOci,omitempty"`


golang coding standard sugguests that acronyms to have a consistent case. For example, EnableOci should be EnabledOCI. See
https://github.com/golang/go/wiki/CodeReviewComments#initialisms
And, I would probably use OCIEnabled as the name of a bool field.

alexmt · 2020-09-18T18:08:54Z

Sorry for delay @haoshuwei . Looking at PR ASAP

alexmt

Sorry for delay again. Added a couple of minor comments. Looks like it is almost ready to merge. Thank you @haoshuwei !

alexmt · 2020-09-22T23:17:42Z

cmd/argocd/commands/app.go

@@ -122,6 +123,9 @@ func NewApplicationCreateCommand(clientOpts *argocdclient.ClientOptions) *cobra.
 	# Create a Helm app from a Helm repo
 	argocd app create nginx-ingress --repo https://kubernetes-charts.storage.googleapis.com --helm-chart nginx-ingress --revision 1.24.3 --dest-namespace default --dest-server https://kubernetes.default.svc

+	# Create a Helm app from a Helm OCI-based repo
+	argocd app create nginx-ingress --repo helm-oci-registry.cn-zhangjiakou.cr.aliyuncs.com --helm-chart acs/nginx-ingress --revision 1.24.3 --dest-namespace default --dest-server https://kubernetes.default.svc


I stuck here for a couple of minutes trying to find the difference with non-oci repository :) . There is difference, right ? In this can we remove this line ?

alexmt · 2020-09-22T23:22:48Z

util/helm/client.go

@@ -106,7 +110,9 @@ func (c *nativeHelmChart) ExtractChart(chart string, version *semver.Version) (s
 	}
 	if !exists {
 		// always use Helm V3 since we don't have chart content to determine correct Helm version
-		helmCmd, err := NewCmdWithVersion(c.repoPath, HelmV3)


nit: is helmCmd := &Cmd{} required. Can you please use helmCmd, err := NewCmdWithVersion(c.repoPath, HelmV3, c.enableOci)?

alexmt · 2020-09-22T23:25:02Z

util/helm/cmd.go

+		args = append(args, "--key-file", filePath)
+	}
+
+	return c.run(args...)


Please add --insecure flag is creds.InsecureSkipVerify is set to true.

alexmt · 2020-09-22T23:25:27Z

util/helm/cmd.go

+	args := []string{"registry", "logout"}
+	args = append(args, repo)
+
+	if creds.CAPath != "" {


Please add --insecure flag is creds.InsecureSkipVerify is set to true

JrCs · 2020-10-26T13:48:44Z

Any news ?

alexmt

The PR contains great changes and implements a very useful feature. It would be a shame to waste the effort.

@haoshuwei implemented 99% of changes with only a few minors review comments. I took the liberty, resolved merged conflicts and reviewer comments in the fork. I hope you don't mind @haoshuwei .

LGTM :)

codecov-io · 2020-10-27T22:26:59Z

Codecov Report

Merging #4018 into master will decrease coverage by 0.38%.
The diff coverage is 15.06%.

@@            Coverage Diff             @@
##           master    #4018      +/-   ##
==========================================
- Coverage   41.37%   40.98%   -0.39%     
==========================================
  Files         126      126              
  Lines       17154    17292     +138     
==========================================
- Hits         7097     7087      -10     
- Misses       9042     9185     +143     
- Partials     1015     1020       +5

Impacted Files	Coverage Δ
cmd/argocd/commands/repo.go	`0.00% <0.00%> (ø)`
pkg/apis/application/v1alpha1/types.go	`57.92% <0.00%> (-0.37%)`	⬇️
util/settings/settings.go	`40.65% <ø> (ø)`
util/argo/argo.go	`59.32% <6.66%> (-5.37%)`	⬇️
util/helm/cmd.go	`25.69% <11.84%> (-13.44%)`	⬇️
util/helm/client.go	`45.03% <21.95%> (-11.01%)`	⬇️
reposerver/repository/repository.go	`60.94% <66.66%> (ø)`
server/application/application.go	`29.10% <100.00%> (ø)`
util/db/repository.go	`63.73% <100.00%> (+0.26%)`	⬆️
controller/appcontroller.go	`50.56% <0.00%> (-1.92%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bf9deb...e40d183. Read the comment docs.

comments had been addressed

haoshuwei changed the title ~~feat:support oci-based helm repository~~ feat: support oci-based helm repository Jul 29, 2020

haoshuwei changed the title ~~feat: support oci-based helm repository~~ feat: Support oci-based helm repository Jul 29, 2020

jannfis previously requested changes Aug 4, 2020

View reviewed changes

jessesuen assigned alexmt Aug 4, 2020

alexmt requested changes Aug 6, 2020

View reviewed changes

cmd/argocd/commands/repo.go Outdated Show resolved Hide resolved

pkg/apis/application/v1alpha1/types.go Outdated Show resolved Hide resolved

feat: Support oci-based helm repository

593a9e0

Signed-off-by: haoshuwei <[email protected]>

haoshuwei requested review from alexmt and jannfis August 17, 2020 03:34

jannfis added this to the v1.8 milestone Sep 8, 2020

wtam2018 reviewed Sep 9, 2020

View reviewed changes

alexmt requested changes Sep 22, 2020

View reviewed changes

alexmt added 3 commits October 27, 2020 13:26

Merge branch 'master' into features/helm-oci-support

335dfe4

Address reviewer comments

0a18bae

Merge branch 'master' into features/helm-oci-support

e40d183

alexmt approved these changes Oct 27, 2020

View reviewed changes

alexmt merged commit 387f775 into argoproj:master Oct 27, 2020

alexmt mentioned this pull request Oct 27, 2020

Support to use Helm 3 with OCI for package distribution #3349

Closed

bergerx mentioned this pull request Oct 28, 2020

Azure Container Registries now supports Helm repos Praqma/helmsman#99

Closed

alxy mentioned this pull request May 4, 2021

Support Helm charts from OCI registries fluxcd/source-controller#124

Closed

unittolabs mentioned this pull request May 31, 2021

Helm OCI repository (ACR) #6365

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support oci-based helm repository #4018

feat: Support oci-based helm repository #4018

haoshuwei commented Jul 29, 2020

codecov-commenter commented Aug 3, 2020 •

edited

Loading

haoshuwei commented Aug 3, 2020

xianlubird commented Aug 4, 2020

jannfis left a comment

haoshuwei commented Aug 11, 2020

haoshuwei commented Aug 25, 2020

SayakMukhopadhyay commented Sep 1, 2020 •

edited

Loading

wtam2018 Sep 9, 2020 •

edited

Loading

alexmt commented Sep 18, 2020

alexmt left a comment

alexmt Sep 22, 2020

alexmt Sep 22, 2020

alexmt Sep 22, 2020

alexmt Sep 22, 2020

JrCs commented Oct 26, 2020

alexmt left a comment

codecov-io commented Oct 27, 2020

feat: Support oci-based helm repository #4018

feat: Support oci-based helm repository #4018

Conversation

haoshuwei commented Jul 29, 2020

codecov-commenter commented Aug 3, 2020 • edited Loading

Codecov Report

haoshuwei commented Aug 3, 2020

xianlubird commented Aug 4, 2020

jannfis left a comment

Choose a reason for hiding this comment

haoshuwei commented Aug 11, 2020

haoshuwei commented Aug 25, 2020

SayakMukhopadhyay commented Sep 1, 2020 • edited Loading

wtam2018 Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

alexmt commented Sep 18, 2020

alexmt left a comment

Choose a reason for hiding this comment

alexmt Sep 22, 2020

Choose a reason for hiding this comment

alexmt Sep 22, 2020

Choose a reason for hiding this comment

alexmt Sep 22, 2020

Choose a reason for hiding this comment

alexmt Sep 22, 2020

Choose a reason for hiding this comment

JrCs commented Oct 26, 2020

alexmt left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 27, 2020

Codecov Report

codecov-commenter commented Aug 3, 2020 •

edited

Loading

SayakMukhopadhyay commented Sep 1, 2020 •

edited

Loading

wtam2018 Sep 9, 2020 •

edited

Loading