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

feat: add skipCrds flag for helm charts #8012

Merged
merged 5 commits into from
Jan 14, 2022

Conversation

patst
Copy link
Contributor

@patst patst commented Dec 22, 2021

Closes #6252

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 included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

This PR allows to skip the CRD creation for helm charts by providing a flag skipCrds .

I removed the additionalTemplateArgs array in the template options. There was only on entry and it was conflicting in turning crd application off.
Helm provides a skip-crds command line argument. But argo-cd uses helm template to generate rendered YAML files which are applied then. Imho skip-crds only works for helm install.

I am unsure if a better name for the flag would be include-crds which is true by default. This would reflect the current behaviour.

From an end user perspective used to the helm install command the skip-crds flag is what we want to have.

* replace additionalTemplateArgs with includeCrds flag
* add testcase for skip crds

Signed-off-by: patst <patrick.steinig@googlemail.com>
* replace additionalTemplateArgs with includeCrds flag
* add testcase for skip crds

Signed-off-by: patst <patrick.steinig@googlemail.com>
# Conflicts:
#	cmd/util/app.go
#	pkg/apis/application/v1alpha1/generated.pb.go
#	pkg/apis/application/v1alpha1/generated.proto
#	pkg/apis/application/v1alpha1/openapi_generated.go
#	pkg/apis/application/v1alpha1/types.go
* replace additionalTemplateArgs with includeCrds flag
* add testcase for skip crds

Signed-off-by: patst <patrick.steinig@googlemail.com>
@jplanza-gr
Copy link

The folks over at prometheus-community decided to take the ball home and close the playground; any help you can provide here would be greatly appreciated.

prometheus-community/helm-charts#1510 (comment)

@sathieu
Copy link
Contributor

sathieu commented Jan 7, 2022

@jplanza-gr Yes. But the correct fix is more #2267.

@jplanza-gr
Copy link

@jplanza-gr Yes. But the correct fix is more #2267.

Thanks, I'll keep an eye on that issue.

@poksiala
Copy link

poksiala commented Jan 8, 2022

Even though the correct fix for kube-prometheus-stack case would be different I feel like this PR in itself would have value as well. I myself like to manage CRDs separately from applications and would love to see this merged. Some other valid uses have been raised in #4723

@patst
Copy link
Contributor Author

patst commented Jan 8, 2022

The folks over at prometheus-community decided to take the ball home and close the playground; any help you can provide here would be greatly appreciated.

prometheus-community/helm-charts#1510 (comment)

I agree with @poksiala . This is not completely related to the prometheus helm chart.
I like the writeup at https://github.com/helm/community/blob/f9e06c16d89ccea1bea77c01a6a96ae3b309f823/architecture/crds.md which covers all pros and cons of different ways of handling CRDs.

I would like to have a separate application to maintain the CRD version and therefore have the possibility to skip the installation via the helm3 crd folder.
This does not work with ArgoCD at the moment because under the hood the helm charts are installed using helm template --include-crds + kubectl apply.

The include-crds is not configurable at the moment which changes with this PR.

util/helm/cmd.go Outdated
@@ -330,8 +331,8 @@ func (c *Cmd) template(chartPath string, opts *TemplateOpts) (string, error) {
for _, v := range opts.APIVersions {
args = append(args, "--api-versions", v)
}
if c.HelmVer.additionalTemplateArgs != nil {
args = append(args, c.HelmVer.additionalTemplateArgs...)
if !opts.SkipCrds {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, thank you for keeping backward compatible behavior!

Copy link
Collaborator

@alexmt alexmt 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!

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #8012 (fece925) into master (99d1dca) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8012      +/-   ##
==========================================
- Coverage   41.54%   41.53%   -0.01%     
==========================================
  Files         174      174              
  Lines       22696    22711      +15     
==========================================
+ Hits         9430     9434       +4     
+ Misses      11925    11923       -2     
- Partials     1341     1354      +13     
Impacted Files Coverage Δ
pkg/apis/application/v1alpha1/types.go 55.32% <ø> (+0.13%) ⬆️
util/helm/helmver.go 80.00% <ø> (ø)
cmd/util/app.go 47.10% <60.00%> (+0.16%) ⬆️
reposerver/repository/repository.go 57.85% <100.00%> (-0.79%) ⬇️
util/helm/cmd.go 28.65% <100.00%> (ø)
controller/sync.go 57.51% <0.00%> (-7.63%) ⬇️
server/application/application.go 31.20% <0.00%> (-1.57%) ⬇️
util/session/sessionmanager.go 68.75% <0.00%> (-1.37%) ⬇️
server/cluster/cluster.go 26.69% <0.00%> (-0.67%) ⬇️
... and 19 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 99d1dca...fece925. Read the comment docs.

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

E2E tests have caught an issue: we still support helm2 . The --skip-crds flag should be added only for Helm3. @patst , can you please fix it.

* make sure include crds is not added for helm2

Signed-off-by: patst <patrick.steinig@googlemail.com>
@patst
Copy link
Contributor Author

patst commented Jan 14, 2022

E2E tests have caught an issue: we still support helm2 . The --skip-crds flag should be added only for Helm3. @patst , can you please fix it.

@alexmt : thanks for your feedback.

I missed the one condition which makes sure -include_crds is not passed for helm2 if skip-crds is unset. I think this is fixed now. Would be great to have the E2E tests run again

@patst
Copy link
Contributor Author

patst commented Jan 14, 2022

the Lint docs failed due to an unrelated issue because https://grafana.apps.argoproj.io/ wasn't reachable at the moment:

> Checking links in docs/operator-manual/metrics.md
> Will allow duplicate links
> Will allow redirects
> Connection timeout = 3s
> White list links matching: 10.97.164.88, 192.168.0.20, argocd.example.com, api.github.com/user, cd.apps.argoproj.io, docker-build, docker-build:443, git.example.com, git.example.com:443, github.com/argoproj/another-private-repo, github.com/argoproj/my-private-repository, github.com/argoproj/other-private-repo, github.com/argoproj/private-repo, github.com/otherproj/another-private-repo, ksonnet.io, raw.githubusercontent.com/argoproj/argo-cd, repo.example.com, repo.example.com:443, server.example.com, kubernetes.default.svc, kubernetes.default.svc:443, localhost:4000, localhost:6443, localhost:8080, localhost:8085, mycluster.com, storage.googleapis.com, ui.argocd.yourorganization.net, ui.argocd.yourorganization.net:443, your-kubernetes-cluster-addr, yourorganization.oktapreview.com, yourorganization.oktapreview.com:443, example-OIDC-provider.com, argocd-dex-server:5556, ghe.example.com, proxy-server-url:8888, keycloak.example.com, argocd.myproject.com, argocd.apps.domain.com, k8sou.apps.192-168-2-144.nip.io, your.argoingress.address, your.domain, external.path.to.argocd.io, my-argo-cd-url, my-login-url, login.microsoftonline.com/xxxxx, accounts.google.com/o/saml2/idp?idpid=Abcde0, accounts.google.com/o/saml2?idpid=Abcde0, sso-url, google-entity-id, github.com/argoproj/argo-cd/manifests/crds, example.com, form.example.com, grafana.example.com, 10.5.39.39, chat.googleapis.com/v1/spaces/, mattermost.example.com, my-grafana.com, github.my-company.com, 1.2.3.4, 2.4.6.8, 9.8.7.6, ghe.example.com, 12.34.567.89, 192.168.99.100:8443, github.com/yourghuser/argo-cd, github.com/argoproj/argo-cd/releases/download/, https://github.com/hayorov/helm-gcs.git; 
> Will not save results
Links to check: 2, 0 white listed
  1. https://github.com/argoproj/argo-cd/blob/master/examples/dashboard.json 
  2. https://grafana.apps.argoproj.io 
Checking URLs: ?✓
Issues :-(
> Links 
  1. [L137]  https://grafana.apps.argoproj.io Failed to open TCP connection to grafana.apps.argoproj.io:443 (Connection refused - connect(2) for "grafana.apps.argoproj.io" port 443) 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to disable installation of v3 helm chart CRDs
5 participants