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.

Sign up for GitHub

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

Jump to bottom

feat(argo-cd): add sync wave delay option for application controller #2915

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(argo-cd): add sync wave delay option for application controller #2915

wants to merge 1 commit into from
+32 −9

Conversation

ssbostan
Copy link

@ssbostan ssbostan commented Sep 10, 2024 •
edited by mbevc1
Loading

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

@ssbostan
Copy link
Author

This commit is related to allowing the user to override the default ARGOCD_SYNC_WAVE_DELAY env. variable.

yu-croco
yu-croco reviewed Sep 10, 2024
View reviewed changes
charts/argo-cd/values.yaml
Comment on lines +251 to +252
# -- Sync wave delay seconds to make delay between applications' sync
controller.sync.wave.delay: 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

📝 default is 2
https://argo-cd.readthedocs.io/en/stable/user-guide/sync-waves/
image

@yu-croco
Copy link
Collaborator

Hi @ssbostan , thank you for your PR, it's almost LGTM.
In order to pass lint, you update README.md.gotmpl and run ./scripts/helm-docs.sh, instead of updating README directly.
Ref: https://github.com/argoproj/argo-helm/blob/main/CONTRIBUTING.md#documentation

@github-actions github-actions bot added size/M and removed size/S labels Sep 10, 2024
@mkilchhofer
Copy link
Member

Hi @ssbostan
Thanks for the contribution.
Is there a reference in upstream manifests of this env variable? I dind't find it there.

Would you mind to also file a PR against upstream and contribute the new setting in the params ConfigMap also there?

@ssbostan
Copy link
Author

Hi @ssbostan Thanks for the contribution. Is there a reference in upstream manifests of this env variable? I dind't find it there.

Would you mind to also file a PR against upstream and contribute the new setting in the params ConfigMap also there?

Yes, please. Could you send me the related information to commit on the upstream as well?

Thank you.

@yu-croco yu-croco added the awaiting-upstream Is waiting for a change upstream to be completed before it can be merged. label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbevc1 mbevc1 Awaiting requested review from mbevc1 mbevc1 is a code owner

@mkilchhofer mkilchhofer Awaiting requested review from mkilchhofer mkilchhofer is a code owner

@jmeridth jmeridth Awaiting requested review from jmeridth jmeridth is a code owner

@pdrastil pdrastil Awaiting requested review from pdrastil pdrastil is a code owner

@tico24 tico24 Awaiting requested review from tico24 tico24 is a code owner

@yu-croco yu-croco Awaiting requested review from yu-croco yu-croco is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
argo-cd awaiting-upstream Is waiting for a change upstream to be completed before it can be merged. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ssbostan @yu-croco @mkilchhofer