fix(analysis)!: evaluate success/failure conditions for non-JSON web metric responses. Fixes #4535

[something-ai-hash]

Summary

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

When json.Unmarshal failed, the code hardcoded AnalysisPhaseSuccessful. Now it passes the response body string to evaluate.EvaluateResult, consistent with the JSON code path.

Fixes #4535

Changes

• metricproviders/webmetric/webmetric.go: evaluate conditions for non-JSON/empty responses instead of always succeeding
• metricproviders/webmetric/webmetric_test.go: add regression tests for empty and plain-text bodies; update existing non-JSON tests to use meaningful conditions

Test plan

• go test ./metricproviders/webmetric/... -count=1
• golangci-lint run ./metricproviders/webmetric/...

Checklist:

• This is a bug fix
• PR title follows conventional commits format and references Analysis Template Web Provider Succeeds Irrespective Of SuccessCondition/FailureCondition When Response Body is Null Or Not JSON #4535
• Commits are signed with DCO (git commit -s)
• CI is green
• Unit tests added/updated
• Tests pass locally
• I have used LLM/AI/Agent tools for this PR but I am responsible for all code of this PR
• I understand what the code does and WHY/HOW it works in several scenarios
• This changes existing behavior for users relying on non-JSON web metrics with conditions that should fail
• My organization is added to USERS.md (N/A)

[github-actions]

Published E2E Test Results

  4 files    4 suites   3h 46m 3s ⏱️
122 tests 113 ✅  7 💤 2 ❌
490 runs  460 ✅ 28 💤 2 ❌

For more details on these failures, see this check.

Results for commit a4385b0.

♻️ This comment has been updated with latest results.

[github-actions]

Published Unit Test Results

2 504 tests   2 504 ✅  3m 22s ⏱️
  130 suites      0 💤
    1 files        0 ❌

Results for commit a4385b0.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.03%. Comparing base (0b529dc) to head (a4385b0).
⚠️ Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4770      +/-   ##
==========================================
- Coverage   85.04%   85.03%   -0.01%     
==========================================
  Files         166      166              
  Lines       19091    19093       +2     
==========================================
  Hits        16236    16236              
- Misses       2001     2002       +1     
- Partials      854      855       +1     

Flag	Coverage Δ
e2e	52.97% <0.00%> (-0.02%)	⬇️
unit-tests	81.37% <100.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

[something-ai-hash]

Hii @kostis-codefresh , can you please review this pr once.

[kostis-codefresh]

Thank you for the contribution.

I have two points though

1. This breaks existing behavior for users that depended on the "bug". If somebody had conditions defined on a non-JSON or empty response body, those conditions are now actually evaluated. If this PR is merged at the very least we should document this in the release notes. FYI @zachaller

Also it is important to note that the original PR at https://github.com/argoproj/argo-rollouts/pull/1212/files has tests marked as When_200Response_And_EmptyBody_Then_Succeed, When_200Response_And_NilBody_Then_Succeed so for some existing users this might seem like breaking documented behavior.

2. The new behavior does something that is not documented anywhere. The web metric documentation says that "The webhook response must return JSON content.". But if this PR is merged this is not true anymore.

If this PR is merged now there is another valid scenario. The web metric call now returns pure text "I am OK" and if successconditionm is return == "I am OK", the analysis will pass even though there is no JSON content anywhere.

I think at the very least the documentation must also be updated to explain that simple text responses are also accepted and show at least one scenario.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[something-ai-hash]

Hii @kostis-codefresh ,
Thanks for the review, I've addressed both points:

1. Release notes / Behavior change
   I've added a BREAKING CHANGE footer to the commit message so it will be included in the release changelog.
   The change is intentional per #4535: successCondition and failureCondition are now evaluated for empty and non-JSON response bodies instead of always returning Successful. Users who relied on the old behavior will need to update their AnalysisTemplates.

2. Documentation
   I've updated docs/analysis/web.md to:

• Remove the statement that the webhook response must return JSON
• Add a Plain text responses section with an example where the webhook returns I am OK and successCondition: result == "I am OK"
• Document that empty bodies assign an empty string to result and conditions are evaluated against it

Kindly review

[kostis-codefresh]

Thank you!