HYPERFLEET-709 - fix: Reject not operator for condition queries#80
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe changes add validation preventing the NOT operator on status condition queries by detecting condition nodes via a new internal helper Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/integration/search_field_mapping_test.go (1)
371-401: Add mixednotcoverage around condition queries.These assertions only cover
notapplied to pure condition expressions. SincesubtreeHasCondition()is specifically walking the whole operand subtree, I'd add one mixed 400 case likenot (labels.region='us-east' AND status.conditions.Ready='True')and one allowed 200 case likestatus.conditions.Ready='True' AND not labels.region='us-west'. That protects both the recursive check and against future over-broad rejection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/search_field_mapping_test.go` around lines 371 - 401, In TestSearchStatusConditionsNotOperator, add two extra assertions to cover mixed operands: (1) a case that expects 400 for a "not" that wraps a mixed subtree containing a condition, e.g. use search "not (labels.region='us-east' AND status.conditions.Ready='True')" and call client.GetClustersWithResponse with the corresponding openapi.SearchParams and assert resp.StatusCode() == http.StatusBadRequest; (2) a case that expects 200 for a condition combined with a negated non-condition, e.g. "status.conditions.Ready='True' AND not labels.region='us-west'", call client.GetClustersWithResponse similarly and assert resp.StatusCode() == http.StatusOK; place these alongside the existing calls so subtreeHasCondition() behavior is exercised for both rejection and allowed patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/search.md`:
- Line 102: The note about supported operators is incomplete: update the
sentence "Only the `=` operator is supported for condition queries" and the
following explicit list to either enumerate all rejected operators (including
`!=`, `<`, `>`, `<=`, `>=`, `in`, `not`) or remove the partial list so it simply
states that only `=` is supported; adjust the text containing "Only the `=`
operator is supported for condition queries" and the parenthetical operator list
to reflect the full set or omit the list to avoid misleading consumers.
---
Nitpick comments:
In `@test/integration/search_field_mapping_test.go`:
- Around line 371-401: In TestSearchStatusConditionsNotOperator, add two extra
assertions to cover mixed operands: (1) a case that expects 400 for a "not" that
wraps a mixed subtree containing a condition, e.g. use search "not
(labels.region='us-east' AND status.conditions.Ready='True')" and call
client.GetClustersWithResponse with the corresponding openapi.SearchParams and
assert resp.StatusCode() == http.StatusBadRequest; (2) a case that expects 200
for a condition combined with a negated non-condition, e.g.
"status.conditions.Ready='True' AND not labels.region='us-west'", call
client.GetClustersWithResponse similarly and assert resp.StatusCode() ==
http.StatusOK; place these alongside the existing calls so subtreeHasCondition()
behavior is exercised for both rejection and allowed patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 110e83e7-b74d-4d6b-991a-42cd9284e48f
📒 Files selected for processing (3)
docs/search.mdpkg/db/sql_helpers.gotest/integration/search_field_mapping_test.go
6fb8fbf to
c31b399
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
d55febc
into
openshift-hyperfleet:main
openshift-hyperfleet#80) ## Summary - Enable `revive` linter with HyperFleet standard rules (`var-naming` and `unexported-return` enabled, `exported` disabled) - Fix pre-existing lint violations surfaced by enabled rules: - `internal/k8s_client/types.go`: rename `GVKFromKindAndApiVersion` to `GVKFromKindAndAPIVersion` (revive var-naming) - `pkg/errors/error.go`: rename struct field `HttpCode` to `HTTPCode` (revive var-naming) - Align `.golangci.yml` settings with architecture standard (`goconst.min-occurrences`, `unused.check-exported`, `unparam.check-exported`, `exhaustive.check-generated`) - Add exclusions for underscore package names (structural renaming is out of scope for this ticket) ## Test plan - [x] `make lint` passes with 0 issues - [x] `make test` passes - all unit tests green - [x] `make test-integration` passes - config-loader and executor suites green (k8s_client fails due to local Docker network issue, maestro skipped on ARM64) ## JIRA https://redhat.atlassian.net/browse/HYPERFLEET-769 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Kubernetes client docs/examples updated to use the clarified GVK API name. * **New Features** * Readiness payload now includes per-check details; server can expose stored config YAML. * Clients gain per-request headers and retry options; container test helpers expose host/env/name. * **Refactor** * Public GVK API renamed for clearer naming. * Service error field renamed to HTTPCode and error structs standardized. * **Chores** * Linting configuration and formatter rules revised. * **Tests** * Tests and examples updated to match renamed APIs and adjusted structs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…HYPERFLEET-556 HYPERFLEET-556 - feat - update mock HyperFleet API to generate more realistic payloads
https://issues.redhat.com/browse/HYPERFLEET-709
Summary by CodeRabbit
Bug Fixes
Documentation
Tests