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(argo-cd): add support for dual stack clusters #2649

Merged
merged 11 commits into from
May 31, 2024

Conversation

M0NsTeRRR
Copy link
Contributor

Add support for dual stack clusters

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

charts/argo-cd/README.md Outdated Show resolved Hide resolved
charts/argo-cd/Chart.yaml Outdated Show resolved Hide resolved
@M0NsTeRRR M0NsTeRRR requested a review from yu-croco April 20, 2024 10:02
@yu-croco
Copy link
Collaborator

@M0NsTeRRR

I have updated the chart changelog with all the changes that come with this pull request according to changelog.

Please update (remove old one and add new one) changelog in Chart.yaml.

Signed-off-by: Ludovic Ortega <[email protected]>
@M0NsTeRRR
Copy link
Contributor Author

Done

yu-croco
yu-croco previously approved these changes Apr 20, 2024
@DrFaust92
Copy link
Contributor

maybe worth to put this in helpers and allow this this to be a common setting on chart with override per service? im assuming whoever wants dual stack probably wants it globally

@mkilchhofer
Copy link
Member

maybe worth to put this in helpers and allow this this to be a common setting on chart with override per service? im assuming whoever wants dual stack probably wants it globally

Maybe the simplest solution would be to move the per-service config inside the global section? Or may there be users only enabling this feature on some components?

@yu-croco yu-croco dismissed their stale review April 21, 2024 23:52

refactoring ideas are in progress.

@M0NsTeRRR
Copy link
Contributor Author

I always let the user choose regarding their setup. In other charts' PRs, they have asked me for per-service or global options as you mentionned, so it depends on your preference.
In my case, both setups are fine, so I have no preference for either.

I'll let you decide which one you choose, and I will update the PR to reflect this :)

@M0NsTeRRR
Copy link
Contributor Author

Hello,
Have you decided on your preferred method for implementing this ?
Regards,

@mkilchhofer
Copy link
Member

Hello, Have you decided on your preferred method for implementing this ? Regards,

I have no strict opinion but I'd go for a global option only. I cannot imagine a use case where you want to have different dualstack configs on component level within Argo CD. ;)

Just for simplicity of the code. If someone needs it in the future, we can extend the code at this point.

@M0NsTeRRR
Copy link
Contributor Author

M0NsTeRRR commented May 17, 2024

Hello, Have you decided on your preferred method for implementing this ? Regards,

I have no strict opinion but I'd go for a global option only. I cannot imagine a use case where you want to have different dualstack configs on component level within Argo CD. ;)

Just for simplicity of the code. If someone needs it in the future, we can extend the code at this point.

No problem, thanks for your answer. I will update the PR to reflect this :)

@github-actions github-actions bot added size/M and removed size/L labels May 17, 2024
@M0NsTeRRR M0NsTeRRR requested a review from yu-croco May 17, 2024 16:09
M0NsTeRRR and others added 2 commits May 17, 2024 18:18
Signed-off-by: Ludovic Ortega <[email protected]>
Signed-off-by: Marco Maurer (-Kilchhofer) <[email protected]>
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.

Thank you for implementing the requested changes 🙏

@M0NsTeRRR
Copy link
Contributor Author

Your welcome, thanks for your review :)

@yu-croco yu-croco merged commit 45ff566 into argoproj:main May 31, 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.

None yet

6 participants