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): add functionality to en/disable argocd-ssh-known-hosts-cm #3083

Merged
merged 3 commits into from
Dec 21, 2024

Conversation

gajicdev
Copy link
Contributor

@gajicdev gajicdev commented Dec 18, 2024

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).

Closes #3082

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.

Hi @gajicdev

thanks for your contribution 🙏 .
Only found 2 small things.

Regarding the "correct" keyword, I'd like to have feedback from other maintainers

@@ -371,6 +371,9 @@ configs:
# SSH known hosts for Git repositories
## Ref: https://argo-cd.readthedocs.io/en/stable/operator-manual/declarative-setup/#ssh-known-host-public-keys
ssh:
# -- Specifies if the argocd-ssh-known-hosts-cm configmap should be created by Helm.
enabled: true
Copy link
Member

@mkilchhofer mkilchhofer Dec 18, 2024

Choose a reason for hiding this comment

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

(Question to other maintainers):
Do we wanna use create as a keyword here to be inline with other ConfigMaps conditionals?

/cc: @yu-croco , @tico24 , @mbevc1

Copy link
Collaborator

Choose a reason for hiding this comment

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

I vote to create to align existing ones. 😀

Copy link
Collaborator

Choose a reason for hiding this comment

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

I concur with @yu-croco

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it to create

charts/argo-cd/Chart.yaml Outdated Show resolved Hide resolved
@gajicdev gajicdev force-pushed the configurable-knownHosts branch from 4b588ae to 0c97774 Compare December 20, 2024 09:07
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.

LGTM
Again, thanks for your contribution @gajicdev 🙏

@yu-croco yu-croco enabled auto-merge (squash) December 21, 2024 11:48
@yu-croco yu-croco merged commit ca63415 into argoproj:main Dec 21, 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.

known-hosts configmap custom values are being deleted after new release
4 participants