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

chore: update gitops engine for force sync option (#5882) #17866

Merged
merged 5 commits into from
Apr 17, 2024

Conversation

kkk777-7
Copy link
Contributor

@kkk777-7 kkk777-7 commented Apr 17, 2024

Update gitops-engine for enable force sync option. Also, add e2e and docs.
Ref: argoproj/gitops-engine#560

Related Issue number
Closes #5882
Closes #7459
Closes #9163

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
@kkk777-7 kkk777-7 requested review from a team as code owners April 17, 2024 07:30
@kkk777-7 kkk777-7 force-pushed the chore/update-gitops-engine branch 3 times, most recently from bb82c02 to 3027116 Compare April 17, 2024 10:04
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
@kkk777-7
Copy link
Contributor Author

@pasha-codefresh , Could you please review when you have some time?

@pasha-codefresh
Copy link
Member

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review

⏱️ Estimated effort to review [1-5]

2, because the changes are straightforward and localized to specific files, involving updates to tests, documentation, and dependency versions.

🧪 Relevant tests

Yes

🔍 Possible issues

No

🔒 Security concerns

No

Code feedback:
relevant filetest/e2e/sync_options_test.go
suggestion      

Consider adding a negative test case to ensure that the Force=true,Replace=true options do not apply incorrectly or affect other resources. This will help in maintaining robustness and catching potential regressions. [important]

relevant linePatchFile("guestbook-ui-deployment.yaml", `[{ "op": "add", "path": "/metadata/annotations", "value": { "argocd.argoproj.io/sync-options": "Force=true,Replace=true" }}]`).

relevant filetest/e2e/sync_options_test.go
suggestion      

Add verification steps after the Sync() operation to check the state of the deployment or other resources to ensure that the Force=true,Replace=true options have the intended effect. This can be done by querying the Kubernetes API or using assertions on expected resource states. [important]

relevant lineSync().

relevant filedocs/user-guide/sync-options.md
suggestion      

Include a warning or note about potential risks or considerations when using Force=true,Replace=true, such as possible disruptions or side effects on running applications. This is important for operational safety and user awareness. [medium]

relevant lineargocd.argoproj.io/sync-options: Force=true,Replace=true

relevant filego.mod
suggestion      

Ensure that the updated version of github.com/argoproj/gitops-engine is thoroughly tested in different environments as it might contain significant changes. Consider setting up a canary deployment or feature flag to mitigate potential risks. [important]

relevant linegit.luolix.top/argoproj/gitops-engine v0.7.1-0.20240416142647-fbecbb86e412


✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

  • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

@@ -165,6 +165,17 @@ metadata:
argocd.argoproj.io/sync-options: Replace=true
```

## Force Sync

For certain resources you might want to delete and recreate. e.g. job resources that should run every time when syncing.
Copy link
Member

Choose a reason for hiding this comment

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

I would also add warning about risks to use replace option, like here
Снимок экрана 2024-04-17 в 17 05 14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pasha-codefresh
thank you comment !
I added warning description of force sync options.

@pasha-codefresh
Copy link
Member

LGTM, just small comment @kkk777-7

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
@pasha-codefresh pasha-codefresh enabled auto-merge (squash) April 17, 2024 16:00
@pasha-codefresh pasha-codefresh merged commit db615ed into argoproj:master Apr 17, 2024
27 checks passed
@tybook
Copy link

tybook commented Apr 24, 2024

Thank you for implementing this! Could we please get this set up for release as part of v2.10.8? Not familiar with the release process for this repo.

@tybook
Copy link

tybook commented May 6, 2024

Gentle nudge :) Could this be included in a patch release soon? This is the last piece needed for ArgoCD to handle jobs for us well. @pasha-codefresh

@eitan-papaya
Copy link

Same, looks like it didn't make it into the 2.11.0 release, was really waiting for this change. Any status update on when should we expect this?

@pasha-codefresh
Copy link
Member

pasha-codefresh commented May 8, 2024

We need cherry-pick it, let me see if i can help here @eitan-papaya

@pasha-codefresh
Copy link
Member

/cherry-pick release-2.10

Copy link

Cherry-pick failed with Merge error db615ed1c55dde361287726eb2748b4a3fb2d0b1 into temp-cherry-pick-8f3a2d-release-2.10

@pasha-codefresh
Copy link
Member

@eitan-papaya @tybook going to cherry-pick it into 2.10, 2.11, let me know if you need it for 2.8 or 2.9

@eitan-papaya
Copy link

@pasha-codefresh
For me 2.10+2.11 is great, don't need for 2.8/2.9. Thank you very much! :)

@digitalyuki
Copy link

@pasha-codefresh , I just found this thread, is there any chance you could also cherry-pick it into the 2.9 release? I've seen your 2 PRs to the 2.10 and 2.11 release branches, it looks straightforward and I could give it a shot if you prefer, but as a non-contributor I suspect it would take me longer to get it all approved / merged in, etc.

@pasha-codefresh
Copy link
Member

@digitalyuki could you please just cherry-pick it? i will make sure that it is reviewed and merged. Let me know if you meed any issues

@digitalyuki
Copy link

@pasha-codefresh here's my attempt to cherry-pick it to for 2.9... Not sure why it failed the CI but committing to this repo is new for me.
#18259

@eitan-papaya
Copy link

eitan-papaya commented May 19, 2024

@pasha-codefresh Thanks again for the quick response. When could we expect a release including this (either 2.10/2.11)? I saw in the docs:

Argo CD patch releases occur on an as-needed basis.

So wanted to understand better if it's expected in the upcoming days or that it can happen sooner given a request.

@pasha-codefresh
Copy link
Member

I will do release during next week, approximately Tuesday

mkieweg pushed a commit to mkieweg/argo-cd that referenced this pull request Jun 11, 2024
…goproj#17866)

* chore: update gitops engine version

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>

* add: e2e and docs for force sync options

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>

* docs: Add warning description of force sync options

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>

---------

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
Co-authored-by: pasha-codefresh <pavel@codefresh.io>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this pull request Jun 16, 2024
…goproj#17866)

* chore: update gitops engine version

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>

* add: e2e and docs for force sync options

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>

* docs: Add warning description of force sync options

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>

---------

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
Co-authored-by: pasha-codefresh <pavel@codefresh.io>
@ishaan-tf
Copy link

@pasha-codefresh Wanted to know, if we included this in a patch release for 2.10/2.11?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants