-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
fix(appset): performProgressiveSyncs only when the applicationset is using it (#15297) #15299
fix(appset): performProgressiveSyncs only when the applicationset is using it (#15297) #15299
Conversation
…using it Signed-off-by: Eric Blackburn <eblackburn@indeed.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #15299 +/- ##
==========================================
- Coverage 49.67% 49.66% -0.01%
==========================================
Files 267 267
Lines 46383 46385 +2
==========================================
Hits 23039 23039
- Misses 21084 21086 +2
Partials 2260 2260
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM.
Is there a way to do an E2E test to verify that the |
I tried to move all the content in this parent progressive sync is On block to its own function and write up unit tests, but I ran out of time. There are just no unit test for this area, so I would have to sit down and design that from scratch. Given my time limitations, I decided to just do the one line fix. Eventually, I want to dedicate more time to bettering progressive sync functionality and add more unit testing. But, it would be a while until I can do that. I can push up a branch to my fork, if someone wants to take a look at my failed attempt at unit testing and tune it up. |
Hi @blakepettersson, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/cherry-pick release-2.9 |
…using it (#15299) Signed-off-by: Eric Blackburn <eblackburn@indeed.com>
/cherry-pick release-2.8 |
/cherry-pick release-2.7 |
…using it (#15299) Signed-off-by: Eric Blackburn <eblackburn@indeed.com>
/cherry-pick release-2.6 |
…using it (#15299) Signed-off-by: Eric Blackburn <eblackburn@indeed.com>
Cherry-pick failed with |
…using it (argoproj#15299) Signed-off-by: Eric Blackburn <eblackburn@indeed.com>
…using it (argoproj#15299) Signed-off-by: Eric Blackburn <eblackburn@indeed.com> Signed-off-by: jmilic1 <70441727+jmilic1@users.noreply.github.com>
…using it (argoproj#15299) Signed-off-by: Eric Blackburn <eblackburn@indeed.com>
…using it (argoproj#15299) Signed-off-by: Eric Blackburn <eblackburn@indeed.com>
…using it (argoproj#15299) Signed-off-by: Eric Blackburn <eblackburn@indeed.com>
…using it (argoproj#15299) Signed-off-by: Eric Blackburn <eblackburn@indeed.com>
Fixes #15297
Updates the if/else statement so that the else statement used to call performProgressiveSyncs is only called when the applicationset is using it.
Note, my build is green. There seems to be some other bugs in Argo ,master, itself unrelated to this change. Those are not in scope for the this PR.
Checklist: