fix(appset): progressive sync loop when applciation has sync errors

[agaudreault]

[bunnyshell[bot]]

:x: Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

• :rocket: /bns:deploy to deploy the environment

[codecov[bot]]

Codecov Report

:x: Patch coverage is 84.43114% with 26 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 61.11%. Comparing base (6f6c39d) to head (fb51ed7). :warning: Report is 45 commits behind head on master.

Files with missing lines	Patch %	Lines
...cationset/controllers/applicationset_controller.go	84.43%	26 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #23187      +/-   ##
==========================================
+ Coverage   60.29%   61.11%   +0.81%     
==========================================
  Files         350      350              
  Lines       59959    60798     +839     
==========================================
+ Hits        36155    37155    +1000     
+ Misses      20909    20748     -161     
  Partials     2895     2895              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[andrii-korotkov]

Need to re-run tests due to flakiness

[agaudreault]

Sort ApplicationStatus by application name so it is consistent Sort the conditions by type so it is consistent

I only see code to sort conditions, and that already existed.

Do we need tests for these?

Sort ApplicationStatus by application name so it is consistent Sort the conditions by type so it is consistent

I only see code to sort conditions, and that already existed.

Do we need tests for these?

There is a call to sort.Slice(applicationSet.Status.ApplicationStatus and sort.Strings(errorApps). The previous code to update conditions was not always calling SetConditions().

[bunnyshell[bot]]

:x: Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

• :rocket: /bns:deploy to deploy the environment

[agaudreault]

@ranakan19 Sorting of the UI is not impacted by changes in this PR. Do you see visual changes with the current version?

[ranakan19]

@ranakan19 Sorting of the UI is not impacted by changes in this PR. Do you see visual changes with the current version?

You're right, this is not impacted by your changes. I was testing with app names containing numericals in my previous setup, so when I switched to different naming conventions for this test, the UI ordering changed and I incorrectly attributed it to the PR. Thanks for the clarification!

[agaudreault]

@ranakan19 Thanks for the review and testing this out :) I just pushed updates to adress 3) 4) and 5). For 2) I don't want to introduce additional refactor to this PR, so I agree that another PR would be better.

[crenshaw-dev]

This was accidentally auto-merged while I was working on some branch protection changes. I've reverted the change on the master branch. Please reopen the PR.