Skip to content

Conversation

@bmbferreira
Copy link
Contributor

  • Introduced ApplicationConditionSyncError to represent synchronization errors.
  • Updated health checker tests to account for the new SyncError condition, increasing the issue count in assertions.
  • Modified health assessment logic to include SyncError in the evaluation of application health.

This should fix #5166. Also noticed this comment saying that we were not sure about all the error states that should be considered here and I think it's important to have SyncError because that's the status we will get if we are using sync waves on argo and one of the "waves" fail (for example running database migrations in the first step, as I reference here).

@bmbferreira bmbferreira requested a review from a team as a code owner November 20, 2025 11:43
@netlify
Copy link

netlify bot commented Nov 20, 2025

Deploy Preview for docs-kargo-io canceled.

Name Link
🔨 Latest commit 61d8829
🔍 Latest deploy log https://app.netlify.com/projects/docs-kargo-io/deploys/691eff26e9e33c00087ea81b

- Introduced ApplicationConditionSyncError to represent synchronization errors.
- Updated health checker tests to account for the new SyncError condition, increasing the issue count in assertions.
- Modified health assessment logic to include SyncError in the evaluation of application health.

Signed-off-by: Bruno Ferreira <[email protected]>
@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.12%. Comparing base (d4facc0) to head (61d8829).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5414   +/-   ##
=======================================
  Coverage   56.12%   56.12%           
=======================================
  Files         411      411           
  Lines       30055    30055           
=======================================
  Hits        16868    16868           
  Misses      12213    12213           
  Partials      974      974           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bmbferreira bmbferreira mentioned this pull request Nov 20, 2025
@antoinedeschenes
Copy link

Perhaps looking at Status.OperationState.Phase rather than individual conditions would be more reliable? That's what was suggested to me when forwarding the issue to the ArgoCD Slack channel

@bmbferreira
Copy link
Contributor Author

Perhaps looking at Status.OperationState.Phase rather than individual conditions would be more reliable? That's what was suggested to me when forwarding the issue to the ArgoCD Slack channel

Probably, but that might require more changes. This seems to be a reasonable change to fix this problem. I would say that SyncError should be considered as a "health error" condition.

Copy link
Contributor

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this, and proposing a fix.

I believe adding this condition could indeed improve things, but I would need a second sign-off on this from either @jessesuen or @krancour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

argocd-update status is Skipped when the Application is in a SyncError condition

3 participants