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: make argocd installation manifests Istio compatible #2784 #3841

Closed
wants to merge 1 commit into from
Closed

feat: make argocd installation manifests Istio compatible #2784 #3841

wants to merge 1 commit into from

Conversation

Cajga
Copy link
Contributor

@Cajga Cajga commented Jun 24, 2020

Checklist:

  • [ X ] 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.
  • [ X ] The title of the PR states what changed and the related issues number (used for the release note).
  • [ X ] 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.
  • [ Partially ] I've signed the CLA and my build is green (troubleshooting builds).

I've signed the CLA no test build was done as I did not touch code.

This PR makes argocd installation manifests compatible with Istio. It implements this request: #2784

In case this is accepted I will make a PR to have 2 examples with Istio ingress:

  • TLS termination at Istio Ingress gateway
  • TLS termination at ArgoCD server (Istio ingress gateway is configured to TLS PASSTHROUGH)

…ce ports to make argocd manifest Istio compatible
@CLAassistant
Copy link

CLAassistant commented Jun 24, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #3841 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3841   +/-   ##
=======================================
  Coverage   42.16%   42.16%           
=======================================
  Files         119      119           
  Lines       17418    17418           
=======================================
  Hits         7345     7345           
  Misses       9130     9130           
  Partials      943      943           

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 28eb286...28942ca. Read the comment docs.

@Cajga
Copy link
Contributor Author

Cajga commented Jun 24, 2020

The failing test is false positive. The manifests files are changed by intention (that is the whole purpose of the PR).

@jannfis
Copy link
Member

jannfis commented Jun 24, 2020

Hi @Cajga, after changing the Kustomization files and running make manifests (or make manifests-local), the resulting installation manifests have to be commited to your branch as well.

@Cajga
Copy link
Contributor Author

Cajga commented Jun 24, 2020

Hi @Cajga, after changing the Kustomization files and running make manifests (or make manifests-local), the resulting installation manifests have to be commited to your branch as well.

Ahh, got it, thanks for the pointer

@Cajga
Copy link
Contributor Author

Cajga commented Jun 24, 2020

So, my PR is good for the normal installation but does not cover the ha manifests properly. I will need to work on it a bit more to have those covered as well.

@Cajga
Copy link
Contributor Author

Cajga commented Jun 25, 2020

Ok, so according this issue Istio started to support the recommended labels as well such as app.kubernetes.io/name and app.kubernetes.io/version.
As ArgoCD already using app.kubernetes.io/name I will close this PR now and open up a new one which adds only app.kubernetes.io/version.

@Cajga Cajga closed this Jun 25, 2020
@Cajga Cajga deleted the istio-compatibility branch June 25, 2020 19:18
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.

3 participants