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

fix(argo-cd): remove secretName for server and applicationSet Certificates #2741

Merged
merged 7 commits into from
Jun 20, 2024

Conversation

erwanval
Copy link
Contributor

@erwanval erwanval commented Jun 4, 2024

argocd-server and argocd-applicationset are expecting a secret with a predefined name, so having the option to set a custom name for the secret is confusing.

Related Issue:
#2724

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

…applicationset are static

Signed-off-by: Erwan Vallienne <erwan@fgtech.fr>
@erwanval erwanval changed the title Fix secretName for server and applicationSet Certificates fix(argo-cd): remove secretName for server and applicationSet Certificates Jun 4, 2024
@jmeridth
Copy link
Member

jmeridth commented Jun 4, 2024

The default values that are assigned match the default values. You do not need to have them in your values.yaml and the default values will be set. I'm not a fan of this PR but await feedback from other maintainers.

Copy link
Member

@mkilchhofer mkilchhofer left a comment

Choose a reason for hiding this comment

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

I have no strict opinion here. I am fine with both.

But the changelog is wrong (you mentioned the wrong parameters).

charts/argo-cd/Chart.yaml Outdated Show resolved Hide resolved
charts/argo-cd/Chart.yaml Outdated Show resolved Hide resolved
@jmeridth
Copy link
Member

jmeridth commented Jun 5, 2024

Closing and reopening to see if DCO kicks off (was off and is on again)

@jmeridth jmeridth closed this Jun 5, 2024
@jmeridth jmeridth reopened this Jun 5, 2024
@jmeridth
Copy link
Member

jmeridth commented Jun 5, 2024

Queue might be backed up still due to dcoapp/app#211. Can try again later.

@jmeridth
Copy link
Member

jmeridth commented Jun 5, 2024

UPDATE: Confirmed. DCO firing again. Thanks @mkilchhofer

@erwanval when you make the changes requested by @mkilchhofer, maybe a new commit will trigger the DCO app. We'll see.

mkilchhofer and others added 2 commits June 5, 2024 21:49
Signed-off-by: Marco Maurer (-Kilchhofer) <mkilchhofer@users.noreply.github.com>
Signed-off-by: Erwan Vallienne <erwan@fgtech.fr>
@erwanval
Copy link
Contributor Author

The default values that are assigned match the default values. You do not need to have them in your values.yaml and the default values will be set. I'm not a fan of this PR but await feedback from other maintainers.

If you keep the default, it works indeed. However, any other values than the default will be ignored, and the application will generate its own self-signed certificate instead. It still starts without error in appearance, but can cause confusion since the exposed cert doesn't match the one you configured.

@mkilchhofer Thanks for fixing the typo in changelog.
Regarding DCO, it seems to have been triggered now, but there is no "Verified" next to my commits somehow? I'm not sure if I'm missing something, it usually works just fine with other repositories.

@mkilchhofer mkilchhofer self-requested a review June 10, 2024 13:05
@mkilchhofer
Copy link
Member

Regarding DCO, it seems to have been triggered now, but there is no "Verified" next to my commits somehow? I'm not sure if I'm missing something, it usually works just fine with other repositories.

DCO is perfectly fine now (green rectangles). It was failing as the DCO GitHub app was not okay.
image

(The verified commits (gpg key or via GitHub GUI) are not required in argo-helm) :)

@mkilchhofer mkilchhofer linked an issue Jun 18, 2024 that may be closed by this pull request
@yu-croco
Copy link
Collaborator

Hi @erwanval , can you please resolve conflict?

Signed-off-by: Erwan Vallienne <135604788+erwanval@users.noreply.github.com>
@erwanval
Copy link
Contributor Author

@yu-croco done, thanks

erwanval and others added 2 commits June 20, 2024 10:03
Signed-off-by: Erwan Vallienne <135604788+erwanval@users.noreply.github.com>
Signed-off-by: Erwan Vallienne <erwan@fgtech.fr>
@yu-croco yu-croco merged commit b0d4648 into argoproj:main Jun 20, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

argocd-sever ignore configured certificate
5 participants