Repository navigation
Conversation
✅ Deploy Preview for pipecd-site canceled.
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation and tests are covered; the only remaining finding is a non-blocking documentation nit.
Review effort: Lite
Findings: 1
What changed in this PR
Fixes HTTP analysis to enforce configured expectedResponse values while preserving status-only checks when unset.
Changes:
- Adds exact response-body validation with a 1 MiB read limit.
- Preserves existing behavior for empty
expectedResponse. - Adds tests for matching, mismatches, limits, and legacy behavior.
- Updates configuration wording.
| File | Summary |
|---|---|
pkg/app/pipedv1/plugin/analysis/config/condition.go |
Documents response validation; a nit remains to clarify exact matching and empty-value behavior in the reference docs. |
pkg/app/pipedv1/plugin/analysis/analysisprovider/http/http.go |
Implements bounded exact response-body validation. |
pkg/app/pipedv1/plugin/analysis/analysisprovider/http/http_test.go |
Tests response validation and size-limit behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
harshrajdebug
left a comment
harshrajdebug
left a comment
There was a problem hiding this comment.
I ran this against a handler that writes its body with json.NewEncoder(w).Encode(...) as its last step, which is a common way for a Go health endpoint to respond. The encoder appends a newline, so the body is {"status":"healthy"}\n and the check fails with the config from #7396, where the value is the single-quoted '{"status":"healthy"}' string. It passes when the value is written as a YAML block scalar, which keeps the trailing newline.
With the default failureLimit of 0, that first mismatch fails the ANALYSIS stage. Anyone who already has expectedResponse set today, where it was ignored, and whose endpoint ends its body with a newline would start seeing failures after upgrading with nothing changed on their side.
Would it make sense to trim trailing newlines from the body before comparing? If exact match is the intended rule, the docs should say so. The Copilot comment about the docs has a "Resolved" reply, but analysis.md isn't changed in this PR, so its expectedResponse row still just reads "Expected response body."
|
Kindly assist in reviewing, as I have made the necessary changes in commit f2b3b4b |
harshrajdebug
left a comment
harshrajdebug
left a comment
There was a problem hiding this comment.
Thanks, f2b3b4b covers it. I re-ran the handler that writes its body with json.NewEncoder(w).Encode(...) as its last step on this head. The single-quoted value fails with "unexpected response body", the value that keeps the trailing newline passes, and an empty expectedResponse still checks only the status code, which is what the analysis.md row now says. The user-facing section already covers configs where expectedResponse was ignored before, so nothing else from my side on the code.
One small thing in the description: the Screenshots section still says the PR does not modify documentation, which changed with f2b3b4b.
|
Kindly assist in reviewing, as I have made the necessary changes to the description. Thanks. |
harshrajdebug
left a comment
harshrajdebug
left a comment
There was a problem hiding this comment.
The description now matches the change, and the code is unchanged since f2b3b4b apart from the merge from master. The http provider tests pass on this head. Looks good to me.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7410 +/- ##
==========================================
+ Coverage 30.95% 30.99% +0.04%
==========================================
Files 602 602
Lines 52862 52873 +11
==========================================
+ Hits 16362 16390 +28
+ Misses 34956 34935 -21
- Partials 1544 1548 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@JbravoI resolve the conflicts |
Signed-off-by: Ewuji John <johnseyi51@gmail.com>
Signed-off-by: Ewuji John <johnseyi51@gmail.com>
Signed-off-by: Ewuji John <johnseyi51@gmail.com>
|
@JbravoI your recent commit is not signed off |
Signed-off-by: Ewuji John <johnseyi51@gmail.com>
|
@mohammedfirdouss Commits now signed off. |
What this PR does
Makes HTTP analysis enforce a configured
expectedResponse. After the HTTP status code succeeds, the provider reads the response body and requires an exact match withexpectedResponse. An emptyexpectedResponsepreserves the existing status-code-only behavior.The response body read is limited to 1 MiB to prevent unbounded memory use.
Why we need it
expectedResponsewas accepted in HTTP analysis configuration but ignored at runtime. As a result, an analysis could succeed when an endpoint returned the expected status code but an unhealthy or otherwise unexpected response body.Which issue(s) this PR fixes
Fixes #7396
Does this PR introduce a user-facing change?
Yes.
expectedResponsenow fail when the response body does not exactly match the configured value. Configurations withoutexpectedResponseretain their current behavior.expectedResponseif they only want to validate the HTTP status code.Screenshots/Videos (for documentation or website changes)
Not applicable. This PR updates configuration-reference documentation only; no screenshots or website changes are needed.