Issue #2261 - Refactor Helm first class support #2364

alexmt · 2019-09-25T19:56:58Z

PR refactors Helm first-class support.

Following major changes are implemented:

added chart field to the application source definition. If chart field is specified the argo cd assumes the application uses Helm repo. This improves user experience and replaces repo type autodetection.
repo server method ListApps support only git repo. ListApps is used only in UI and UI is different in case of Helm.
repo server method GetRevisionMetadata support only git repo. Git commit metadata is different from helm chart metadata. There is no author, tags, message message in helm chart metadata.
introduced helm specific method GetHelmCharts which is used to power app creation UI
implements proper application creation UI with both chart and version auto-completion:

Following issues are solved:

All repo server methods unnecessary lock repository even if method result is available in cache: https://github.com/argoproj/argo-cd/blob/master/reposerver/repository/repository.go#L115
GetRevisionMetadata caches metadata of an ambiguous revision instead of resolved https://github.com/argoproj/argo-cd/blob/master/reposerver/repository/repository.go#L629
Improves Helm apps performance.
- Currently repo server downloads chart every time (unless result is cached). Refactored version download chart once and save it on disk. Charts use semantic versioning so it is safe to assume that chart don't chart.
- Currently repo server executes only one operation per helm repo. The majority of public helm charts are in single repo: https://kubernetes-charts.storage.googleapis.com so this approach won't scale. Refactored version Instead of holding lock on the whole repo refactored version locks per chart/version.

codecov · 2019-09-25T20:32:42Z

Codecov Report

Merging #2364 into master will decrease coverage by 1.54%.
The diff coverage is 8.68%.

@@            Coverage Diff             @@
##           master    #2364      +/-   ##
==========================================
- Coverage    38.8%   37.25%   -1.55%     
==========================================
  Files         109      103       -6     
  Lines       14197    14146      -51     
==========================================
- Hits         5509     5270     -239     
- Misses       7970     8203     +233     
+ Partials      718      673      -45

Impacted Files	Coverage Δ
pkg/apis/application/v1alpha1/types.go	`53.18% <0%> (-0.22%)`	⬇️
server/server.go	`51.48% <0%> (+0.43%)`	⬆️
reposerver/repository/helm.go	`0% <0%> (ø)`
util/git/git.go	`78.37% <0%> (-15.18%)`	⬇️
cmd/argocd/commands/app.go	`1.82% <0%> (-0.01%)`	⬇️
util/argo/argo.go	`25% <0%> (-3.51%)`	⬇️
util/helm/cmd.go	`26.54% <0%> (-10.96%)`	⬇️
reposerver/repository/repository.go	`25.89% <0.54%> (-21.43%)`	⬇️
util/git/client.go	`51.88% <100%> (-0.6%)`	⬇️
util/cache/cache.go	`20.53% <31.57%> (-0.19%)`	⬇️
... and 5 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 32f825e...a9872aa. Read the comment docs.

alexec

I've only some minor comments really. A couple of areas to watch out for:

I found that there was not much in the way of tests around caching, so be wary of changes in those areas.
I feel more confident about performance with these changes as we can optimize where we need to.

alexec · 2019-09-25T20:10:18Z

pkg/apis/application/v1alpha1/types.go

@@ -110,6 +110,12 @@ type ApplicationSource struct {
 	Directory *ApplicationSourceDirectory `json:"directory,omitempty" protobuf:"bytes,10,opt,name=directory"`
 	// ConfigManagementPlugin holds config management plugin specific options
 	Plugin *ApplicationSourcePlugin `json:"plugin,omitempty" protobuf:"bytes,11,opt,name=plugin"`
+	// Chart is a Helm chart name
+	Chart string `json:"chart,omitempty" protobuf:"bytes,12,opt,name=chart"`


one thought, what about

spec: source: helm: chart: my-chart

I like it in general, but spec still looks unclean if we introduce helm and keep path and targetRevision.

I would deprecate and migration chart, targetRevision and path, targetRevision to

# one of source: helm: chart: my-chart version: my-version git: path: my-path revision: my-revision

Is it ok to create ticket and migrate both when we are ready?

sure, I still think we need more user feedback on this feature as a priority over perfection

alexec · 2019-09-25T20:12:00Z

reposerver/repository/helm.go

+	s.repoLock.Lock(repoPath)
+	defer s.repoLock.Unlock(repoPath)
+
+	err := os.Mkdir(repoPath, 0700)


I seem to recall someone concerned about permissions, is 0700 correct?

I think that was this comment: #1865 (comment) . The suggestion was to change 0777 to 0700

reposerver/repository/helm.go

alexec · 2019-09-25T20:14:13Z

reposerver/repository/repository.proto

+
+message HelmChart {
+    string name = 1;
+    repeated string versions = 2;


alexec · 2019-09-25T20:24:14Z

server/repository/repository.go

@@ -158,26 +156,38 @@ func (s *Server) GetAppDetails(ctx context.Context, q *repositorypkg.RepoAppDeta
 		return nil, err
 	}
 	return repoClient.GetAppDetails(ctx, &apiclient.RepoServerAppDetailsQuery{
-		Repo:     repo,


I've found a few places in the code where you have to pass values around for it to work correctly, and where unit tests won't go red. This could be one of the places.

Thanks for pointing to this method. Noticed that we don't set Plugins field here and discovered at field is not required at all. Removed it.

util/helm/cmd.go

util/helm/index.go

alexec · 2019-09-25T20:33:11Z

cmd/argocd/commands/app.go

@@ -593,7 +596,8 @@ type appOptions struct {

 func addAppFlags(command *cobra.Command, opts *appOptions) {
 	command.Flags().StringVar(&opts.repoURL, "repo", "", "Repository URL, ignored if a file is set")
-	command.Flags().StringVar(&opts.appPath, "path", "", "Path in repository to the ksonnet app directory, ignored if a file is set")
+	command.Flags().StringVar(&opts.appPath, "path", "", "Path in repository to the app directory, ignored if a file is set")
+	command.Flags().StringVar(&opts.chart, "chart", "", "Chart is a Helm Chart name")


maybe --helm-chart?

agree. This is more in line with other helm related flags. Changed

alexec · 2019-09-25T20:46:20Z

reposerver/repository/repository.proto

-message KsonnetAppDetailsQuery {
-    string environment = 1;
+    github.com.argoproj.argo_cd.pkg.apis.application.v1alpha1.ApplicationSource source = 2;
+    repeated github.com.argoproj.argo_cd.pkg.apis.application.v1alpha1.Repository repos = 3;


alexec · 2019-09-25T20:46:54Z

util/helm/repo/index_test.go

@@ -1,24 +0,0 @@
-package repo


don't you want these tests, but move them?

oh, nice catch. thank you!

alexmt · 2019-09-26T00:57:39Z

@alexec , do you want to make any other changes in this PR? Can I get approval please?

alexec · 2019-09-26T19:03:03Z

VERSION

@@ -1 +1 @@
-1.1.2


what's this?

This file is used to override the value of a version constant in https://github.com/argoproj/argo-cd/blob/master/common/version.go#L11

alexmt requested review from alexec and jessesuen September 25, 2019 19:56

alexmt force-pushed the 2261-helm-improvements branch from bf1193f to 45d097c Compare September 25, 2019 20:32

alexmt force-pushed the 2261-helm-improvements branch from 45d097c to db915a2 Compare September 25, 2019 20:43

alexec reviewed Sep 25, 2019

View reviewed changes

Issue argoproj#2261 - Refactor Helm first class support

3cd57dd

alexmt force-pushed the 2261-helm-improvements branch from db915a2 to 3cd57dd Compare September 25, 2019 21:09

Address code review comments

2d2d85c

alexmt force-pushed the 2261-helm-improvements branch from e132474 to 2d2d85c Compare September 25, 2019 22:39

alexmt requested a review from alexec September 25, 2019 22:48

Remove unused field from GetAppDetails method

a9872aa

alexmt force-pushed the 2261-helm-improvements branch from 9c3afed to a9872aa Compare September 25, 2019 23:26

alexec reviewed Sep 26, 2019

View reviewed changes

alexec approved these changes Sep 26, 2019

View reviewed changes

alexmt merged commit 0f7e9a1 into argoproj:master Sep 26, 2019

alexmt deleted the 2261-helm-improvements branch September 26, 2019 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #2261 - Refactor Helm first class support #2364

Issue #2261 - Refactor Helm first class support #2364

alexmt commented Sep 25, 2019

codecov bot commented Sep 25, 2019 •

edited

Loading

alexec left a comment

alexec Sep 25, 2019

alexmt Sep 25, 2019

alexec Sep 25, 2019

alexec Sep 25, 2019

alexmt Sep 25, 2019

alexec Sep 25, 2019

alexec Sep 25, 2019

alexmt Sep 25, 2019

alexec Sep 25, 2019

alexmt Sep 25, 2019

alexec Sep 25, 2019

alexec Sep 25, 2019

alexmt Sep 25, 2019

alexmt commented Sep 26, 2019

alexec Sep 26, 2019

alexmt Sep 26, 2019

		@@ -1 +1 @@
		1.1.2

Issue #2261 - Refactor Helm first class support #2364

Issue #2261 - Refactor Helm first class support #2364

Conversation

alexmt commented Sep 25, 2019

codecov bot commented Sep 25, 2019 • edited Loading

Codecov Report

alexec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmt commented Sep 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 25, 2019 •

edited

Loading