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

fix(controller): copy visited map (#11699) #12667

Merged
merged 3 commits into from
May 14, 2024
Merged

fix(controller): copy visited map (#11699) #12667

merged 3 commits into from
May 14, 2024
+224 −8

Conversation

ornew
Copy link
Contributor

@ornew ornew commented Mar 1, 2023 •
edited
Loading

This commit fixed an issue #11699 that caused a warning even if the cycle didn't exist. Fix false cycle discovery by copying the visited resource map before recursively calling of getAppRecursive

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).
  • 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.
  • Optional. My organization is added to USERS.md.
  • 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).

Sorry, something went wrong.

@ornew ornew force-pushed the fix-incorrect-circular-warning-11699 branch 4 times, most recently from 9c8b339 to 56daa69 Compare March 2, 2023 00:57
@codecov
Copy link

codecov bot commented Mar 2, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 49.59%. Comparing base (f87897c) to head (07f1857).
Report is 161 commits behind head on master.

❗ Current head 07f1857 differs from pull request most recent head d722a84. Consider uploading reports for the commit d722a84 to get more accurate results

Files Patch % Lines
controller/cache/cache.go 84.61% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #12667      +/-   ##
==========================================
- Coverage   49.73%   49.59%   -0.15%     
==========================================
  Files         274      269       -5     
  Lines       48948    46588    -2360     
==========================================
- Hits        24343    23104    -1239     
+ Misses      22230    21211    -1019     
+ Partials     2375     2273     -102

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@crenshaw-dev
Copy link
Member

Thanks for the PR @ornew! Is this something we can unit test for?

@ornew
Copy link
Contributor Author

ornew commented Mar 7, 2023

Yes, as soon as I get the time I will add the unit test.

This was referenced Jun 21, 2023
Closed
Closed
@Lavanya-Anbalagan
Copy link

we are facing the same cyclic Dependency issue, Can we get this merged?.

@crenshaw-dev
Copy link
Member

@Lavanya-Anbalagan would you have time to contribute a unit test?

@ornew
Copy link
Contributor Author

ornew commented Jul 22, 2023

I apologize for neglecting this task.
I will update the branch and add unit tests as my hands are free today.

@ornew ornew force-pushed the fix-incorrect-circular-warning-11699 branch 2 times, most recently from c9a290a to cbd1a40 Compare July 22, 2023 09:59
@ornew
Copy link
Contributor Author

ornew commented Jul 22, 2023

@crenshaw-dev (cc @Lavanya-Anbalagan)
I rebased the branch to master and added unit tests.

@ornew ornew force-pushed the fix-incorrect-circular-warning-11699 branch 2 times, most recently from ebc857e to 7b83709 Compare August 12, 2023 09:26
@crenshaw-dev crenshaw-dev changed the title fix: copy visited map #11699 fix(controller): copy visited map (#11699) Oct 3, 2023
@ornew ornew force-pushed the fix-incorrect-circular-warning-11699 branch from 7b83709 to 07f1857 Compare October 19, 2023 05:17
@ornew ornew requested a review from a team as a code owner October 19, 2023 05:17
@ornew ornew force-pushed the fix-incorrect-circular-warning-11699 branch from 07f1857 to 50a1d4d Compare October 19, 2023 16:05
@ornew
fix: copy visited map argoproj#11699
d42e881 
This commit fixed an issue argoproj#11699 that caused a warning even if the cycle didn't exist.
Fix false cycle discovery by copying the visited resource map before recursively calling of getAppRecursive.

Fixes argoproj#11699

Signed-off-by: Arata Furukawa <old.river.new@gmail.com>
@ornew ornew force-pushed the fix-incorrect-circular-warning-11699 branch from 50a1d4d to d42e881 Compare October 26, 2023 04:30
gdsoumya
gdsoumya approved these changes May 13, 2024
View reviewed changes
Copy link
Member

@gdsoumya gdsoumya left a comment

Choose a reason for hiding this comment

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

LGTM

alexmt
alexmt approved these changes May 14, 2024
View reviewed changes
Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you

@alexmt alexmt merged commit 36ff5cf into argoproj:master May 14, 2024
27 checks passed
@blakepettersson
Copy link
Member

/cherry-pick release-2.10

@blakepettersson
Copy link
Member

/cherry-pick release-2.11

gcp-cherry-pick-bot bot pushed a commit that referenced this pull request May 14, 2024
@ornew @blakepettersson
fix: copy visited map #11699 (#12667)
Loading
Loading status checks…
9ac30a2 
This commit fixed an issue #11699 that caused a warning even if the cycle didn't exist.
Fix false cycle discovery by copying the visited resource map before recursively calling of getAppRecursive.

Fixes #11699

Signed-off-by: Arata Furukawa <old.river.new@gmail.com>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
@gcp-cherry-pick-bot gcp-cherry-pick-bot bot mentioned this pull request May 14, 2024
Merged
gcp-cherry-pick-bot bot pushed a commit that referenced this pull request May 14, 2024
@ornew @blakepettersson
fix: copy visited map #11699 (#12667)
Loading
Loading status checks…
e900054 
This commit fixed an issue #11699 that caused a warning even if the cycle didn't exist.
Fix false cycle discovery by copying the visited resource map before recursively calling of getAppRecursive.

Fixes #11699

Signed-off-by: Arata Furukawa <old.river.new@gmail.com>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
@gcp-cherry-pick-bot gcp-cherry-pick-bot bot mentioned this pull request May 14, 2024
Merged
pasha-codefresh pushed a commit that referenced this pull request May 20, 2024
@gcp-cherry-pick-bot @ornew @blakepettersson
fix: copy visited map #11699 (#12667) (#18219)
Loading
Loading status checks…
786e141 
This commit fixed an issue #11699 that caused a warning even if the cycle didn't exist.
Fix false cycle discovery by copying the visited resource map before recursively calling of getAppRecursive.

Fixes #11699

Signed-off-by: Arata Furukawa <old.river.new@gmail.com>
Co-authored-by: Arata Furukawa <old.river.new@gmail.com>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
pasha-codefresh pushed a commit that referenced this pull request May 20, 2024
@gcp-cherry-pick-bot @ornew @blakepettersson
fix: copy visited map #11699 (#12667) (#18220)
Loading
Loading status checks…
5425568 
This commit fixed an issue #11699 that caused a warning even if the cycle didn't exist.
Fix false cycle discovery by copying the visited resource map before recursively calling of getAppRecursive.

Fixes #11699

Signed-off-by: Arata Furukawa <old.river.new@gmail.com>
Co-authored-by: Arata Furukawa <old.river.new@gmail.com>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
mkieweg pushed a commit to mkieweg/argo-cd that referenced this pull request Jun 11, 2024
@ornew @blakepettersson @mkieweg
fix: copy visited map argoproj#11699 (argoproj#12667)
6886610 
This commit fixed an issue argoproj#11699 that caused a warning even if the cycle didn't exist.
Fix false cycle discovery by copying the visited resource map before recursively calling of getAppRecursive.

Fixes argoproj#11699

Signed-off-by: Arata Furukawa <old.river.new@gmail.com>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this pull request Jun 16, 2024
@ornew @blakepettersson @Hariharasuthan99
fix: copy visited map argoproj#11699 (argoproj#12667)
899cb76 
This commit fixed an issue argoproj#11699 that caused a warning even if the cycle didn't exist.
Fix false cycle discovery by copying the visited resource map before recursively calling of getAppRecursive.

Fixes argoproj#11699

Signed-off-by: Arata Furukawa <old.river.new@gmail.com>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
jsolana pushed a commit to jsolana/argo-cd that referenced this pull request Jul 24, 2024
@ornew @blakepettersson
fix: copy visited map argoproj#11699 (argoproj#12667)
4997816 
This commit fixed an issue argoproj#11699 that caused a warning even if the cycle didn't exist.
Fix false cycle discovery by copying the visited resource map before recursively calling of getAppRecursive.

Fixes argoproj#11699

Signed-off-by: Arata Furukawa <old.river.new@gmail.com>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
Signed-off-by: Javier Solana <javier.solana@cabify.com>
Signed-off-by: Javier Solana <javier.solana@cabify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexmt alexmt

@gdsoumya gdsoumya

Assignees
No one assigned
Labels
ready-for-review
Projects
Argo CD Review
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ornew @crenshaw-dev @Lavanya-Anbalagan @blakepettersson @alexmt @gdsoumya