Skip to content

fix(ui): Cannot view AnalysisRun result in UI when web provider returns text instead of JSON - #6758

Merged
Marvin9 merged 2 commits into
akuity:mainfrom
jmurret:jm/4630
Aug 25, 2026
Merged

fix(ui): Cannot view AnalysisRun result in UI when web provider returns text instead of JSON#6758
Marvin9 merged 2 commits into
akuity:mainfrom
jmurret:jm/4630

Conversation

@jmurret

@jmurret jmurret commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Description

Closes #4630

This PR represents the UI extension of argoproj/argo-rollouts#4770 that addressed:

Fixes a bug where the web metric provider always returned Successful for empty or non-JSON HTTP response bodies, without evaluating successCondition / failureCondition.

text was always supported but the successCondition was ignored and always returned as successful.

Checklist

Eligibility

  • Linked to an existing issue with no blocking labels (kind/proposal, needs discussion, needs research, maintainer only, area/security, size/large, size/x-large, size/xx-large).
  • Changes documentation only.
  • Changes ten lines or fewer.

Quality

  • Adds or updates corresponding tests.
  • Adds or updates corresponding documentation.

AI Use Disclosure

This PR was written:

  • By a human without AI assistance.
  • By a human with AI assistance. A human has reviewed every line prior to opening the PR.
  • By an AI with human supervision. A human has reviewed every line prior to opening the PR.
  • Entirely by an AI. No human has reviewed this prior to opening the PR.

Sign-Off

All commits:

  • Are signed off by their author (git commit -s) (required)
  • Are cryptographically signed (git commit -S) (encouraged)

@jmurret
jmurret requested a review from a team as a code owner August 4, 2026 01:30
@netlify

netlify Bot commented Aug 4, 2026

Copy link
Copy Markdown

Deploy Preview for docs-kargo-io ready!

Name Link
🔨 Latest commit aa4c260
🔍 Latest deploy log https://app.netlify.com/projects/docs-kargo-io/deploys/6a8c661af32abf0008e3d93c
😎 Deploy Preview https://deploy-preview-6758.docs.kargo.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@kargo-governance-bot kargo-governance-bot Bot added area/ui Affects the UI kind/bug Something isn't working as intended; If unsure that something IS a bug, start a discussion instead needs/priority Priority has not yet been determined; a good signal that maintainers aren't fully committed labels Aug 4, 2026
@Marvin9

Marvin9 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

On the surface from what you shared upstream, it looks like instead of failing, if it returns text, we would want to parse it as text and compare against?

This PR silently drops it. I didn't look in depth but I will

@hdost

hdost commented Aug 18, 2026

Copy link
Copy Markdown

Perhaps #6799 looks like it's trying to fix the same bug

} catch {
return {
canChart: false,
tableValue: value

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok I finally got a chance to take a look at this issue.

https://github.com/argoproj/argo-rollouts/blob/master/ui/src/app/components/analysis-modal/transforms.ts#L628

upstream discards it, I don't know motivation behind it but does it make sense NOT TO discard it. Maybe its also better to show something then nothing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Marvin9 thank you for pointing out the upstream. I've added some reasoning in a comment of why this should match the behavior in rollouts. let me know what you think, but it is basically:

  • one failed measurement shouldn't make cause the whole chart to not be rendered
  • allowing any string of any length to be displayed in a table is risky

@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 31.04%. Comparing base (0922666) to head (aa4c260).
⚠️ Report is 47 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6758      +/-   ##
==========================================
+ Coverage   30.75%   31.04%   +0.29%     
==========================================
  Files         821      835      +14     
  Lines       80358    81453    +1095     
==========================================
+ Hits        24712    25290     +578     
- Misses      54187    54692     +505     
- Partials     1459     1471      +12     

☔ View full report in Codecov by Harness.
📢 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.

@Marvin9

Marvin9 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

@jmurret could you please sign and amend all commits in this PR? And fix the lint issue?

Ready to merge after that

@jmurret
jmurret force-pushed the jm/4630 branch 2 times, most recently from 916ee2b to 393def4 Compare August 23, 2026 15:25
@jmurret

jmurret commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Marvin9 . I consolidated to one signed commit. also the lint error was fixed in the last revision and make type-checkui runs locally without error, so should be 👌 when you run CI.

REVISED: I saw the lint-ui after and added this commit to fix those 2 issues: 5cbc247

@Marvin9

Marvin9 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Sorry @jmurret DCO is failing, you would need to amend signed commits

…ns text instead of JSON

Signed-off-by: John Murret <jmurret@yahoo.com>
Signed-off-by: John Murret <jmurret@yahoo.com>
@jmurret

jmurret commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

@Marvin9 So sorry about that. This is resolved and DCO is passing.

@Marvin9 Marvin9 added this to the v1.11.3 milestone Aug 25, 2026
@Marvin9
Marvin9 added this pull request to the merge queue Aug 25, 2026
Merged via the queue into akuity:main with commit d7ecf09 Aug 25, 2026
18 checks passed
@akuitybot

Copy link
Copy Markdown
Collaborator

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

Labels

area/ui Affects the UI backport/release-1.11 kind/bug Something isn't working as intended; If unsure that something IS a bug, start a discussion instead needs/priority Priority has not yet been determined; a good signal that maintainers aren't fully committed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot view AnalysisRun result in UI - not a valid JSON

4 participants