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 appHardResyncPeriod option for controller #1320

Conversation

jmeridth
Copy link
Member

@jmeridth jmeridth commented Jun 8, 2022

Base on this PR upstream argoproj/argo-cd#8928 we can
now provide a hard resync value to the ArgoCD Application Controller

Signed-off-by: jmeridth jmeridth@gmail.com

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

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

Changes are automatically published when merged to main. They are not published on branches.

@jmeridth jmeridth force-pushed the jm/add-hard-resync-option-for-argo-cd-controller branch from 2a7d0d6 to 5f02e57 Compare June 8, 2022 17:05
@jmeridth
Copy link
Member Author

jmeridth commented Jun 8, 2022

@pasha-codefresh do you have any concerns about this change. I ask since you did the PR argoproj/argo-cd#8928. Thank you in advance if you take a look. Cheers.

@jmeridth
Copy link
Member Author

jmeridth commented Jun 8, 2022

Error: unknown flag: --app-hard-resync looks like that this hasn't been released with argo-cd yet. Whoops. This will be included in argo-cd 2.4.0

@jmeridth jmeridth marked this pull request as draft June 8, 2022 17:24
@pdrastil
Copy link
Member

This will be available via timeout.hard.reconciliation parameter in #1267 that exposes all currently known parameters / arguments. Linking just for reference so this is not merged into PR by mistake.

@mkilchhofer mkilchhofer changed the title feat(argo-cd) Add appHardResyncPeriod option for controller feat(argo-cd): Add appHardResyncPeriod option for controller Jun 17, 2022
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@pasha-codefresh
Copy link
Member

@jmeridth it looks good to me

@jmeridth jmeridth force-pushed the jm/add-hard-resync-option-for-argo-cd-controller branch from 41c3fad to 1c0a27f Compare August 18, 2022 19:52
@jmeridth jmeridth marked this pull request as ready for review August 18, 2022 19:58
@jmeridth
Copy link
Member Author

@pasha-codefresh rebased onto main. Ready to go.

I'm also looking at @pdrastil's PR #1267. Don't want to create a regression here. @pdrastil you ok with this possibly merging and removing from your PR as you're going with timeout.hard.reconciliation? I see you're trying to break that PR apart currently.

@jmeridth jmeridth requested a review from pdrastil August 18, 2022 20:17
@pdrastil
Copy link
Member

pdrastil commented Aug 18, 2022

@jmeridth I'm ok with this PR but when mine will go through this method will be deprecated and later removed. Cobra framework always process command line arguments before parsing the env vars.

Edit: Plan that I've told @mkilchhofer on Slack is to push most backward compatible changes. Then add config for env vars and then remove everything deprecated in next major version.

@jmeridth jmeridth force-pushed the jm/add-hard-resync-option-for-argo-cd-controller branch 2 times, most recently from 28ae358 to d6e6d05 Compare August 25, 2022 13:33
@jmeridth
Copy link
Member Author

@yu-croco @pdrastil @pasha-codefresh can I please snag some 👀 on this one? @pdrastil asking again, is this worth it if #1267 is close to merge? I don't want to add unnecessary work for that PR if this is just going to be deprecated. Thanks.

@pdrastil
Copy link
Member

@jmeridth - I will be splitting refactoring / argo-cmd-params today as all minor features / enhancements are in.

@jmeridth
Copy link
Member Author

jmeridth commented Aug 25, 2022

@pdrastil once I see those PRs, I'm comfortable closing this one. @pasha-codefresh are you okay with this? I know this PR complements a change you made to argo-cd.

@pdrastil
Copy link
Member

@jmeridth You can start looking at #1267. I've removed all the refactoring and did my best to keep it backwards compatible.

@jmeridth
Copy link
Member Author

@pdrastil so based on this my PR is still "relevant" as this hard resync can be a DEPRECATED option for a bit. I see you're including config.params.timeout.hard.reconcilliation for future usage.

I'm willing to close this PR and then create a PR for the deprecated option once yours merges. Or we can merge this and you can add the deprecated option for hard resync to your PR? 🤷

Base on this PR upstream argoproj/argo-cd#8928 we can
now provide a hard resync value to the ArgoCD Application Controller

Signed-off-by: JM <jmeridth@gmail.com>
@jmeridth jmeridth force-pushed the jm/add-hard-resync-option-for-argo-cd-controller branch from d6e6d05 to 680a76d Compare August 26, 2022 15:18
@pdrastil
Copy link
Member

@jmeridth or we can merge this and I can make it deprecated when the configs.params is merged. Up to you - I'm fine with both ways.

@jmeridth
Copy link
Member Author

@yu-croco @mkilchhofer could you please review this? once reviewed we can merge and then @pdrastil can add this flag to his PR (#1267) as a deprecated option. Thank you in advance.

@jmeridth jmeridth merged commit dd4fdef into argoproj:main Aug 26, 2022
@jmeridth jmeridth deleted the jm/add-hard-resync-option-for-argo-cd-controller branch August 26, 2022 16:15
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.

4 participants