HYPERFLEET-1088 - feat: add channel/version E2E tests with typed partner client#110
Merged
openshift-merge-bot[bot] merged 1 commit intoJun 3, 2026
Conversation
kuudori
changed the title
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR establishes a generic resource CRUD abstraction in the client library and applies it to channel and version APIs. It defines Resource and ResourceList models with JSON serialization, request payloads for create/patch operations, and core methods (Create, Get, List, Delete, Patch) that delegate to a shared HTTP handler. The client struct is extended to store httpClient and a normalized baseURL. Channel and version APIs delegate to these generic resource methods using endpoint-specific paths. A test helper is added for best-effort channel cleanup that recursively deletes versions before the channel. E2E test suites cover full CRUD lifecycle for channels and versions, enforce delete restrictions (409 Conflict when versions exist), validate filtering and uniqueness constraints, and use templated JSON payloads for resource creation. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/channel/delete_restrict.go`:
- Around line 27-33: The test currently dereferences ch.Id and ver.Id without
nil guards which can cause panics; update the
CreateChannelFromPayload/CreateVersionFromPayload result handling by asserting
returned objects and their Id fields are non-nil (e.g.,
Expect(ch).NotTo(BeNil()) and Expect(ch.Id).NotTo(BeNil()) before assigning
channelID = *ch.Id) and do the same for ver and versionID, so failures produce
readable test expectations instead of panics when CreateChannelFromPayload or
CreateVersionFromPayload returns partial objects.
In `@Makefile`:
- Around line 48-53: The Makefile rule that downloads OpenAPI specs (the loop
reading openapi/specs.lock in the update-specs target) currently saves remote
artifacts without integrity checks; update the loop to read an expected checksum
(e.g., add a checksum column to openapi/specs.lock) and after curl -sfL
downloads each asset to a temporary file, verify it with a checksum tool
(sha256sum -c or openssl dgst -sha256) before moving it to
openapi/$$name-openapi.yaml; on mismatch, print a clear error (e.g., "CHECKSUM
FAIL: $$name") and exit 1 to prevent replacing local specs with tampered
artifacts.
- Around line 87-91: The clean target currently deletes committed OpenAPI source
files (openapi/core-openapi.yaml and openapi/partner-openapi.yaml), causing
builds to fail; update the clean rule (target "clean") to stop removing those
committed spec files and only remove generated/derived artifacts (e.g., keep rm
-rf $(BIN_DIR) $(OUTPUT_DIR) and rm -rf pkg/api/openapi, and if you need to
remove generated OpenAPI outputs use a clearly named pattern like
openapi/generated-*.yaml or a dedicated generated/ directory) so that source
spec files are preserved.
In `@openapi/core-openapi.yaml`:
- Around line 1-55: The OpenAPI document lacks a global security requirement so
endpoints can accidentally be public; add a document-level security section at
the root of this spec that requires BearerAuth by default (the same scheme
referenced in operation-level security), e.g., add a top-level "security" array
requiring BearerAuth so operations like getClusters (operationId getClusters)
inherit authentication automatically and only explicitly override it where
public access is intended.
- Around line 1268-1271: The OpenAPI schema for AdapterStatusCreateRequest
leaves the conditions array unbounded; update the AdapterStatusCreateRequest
schema's conditions property to enforce size limits (e.g., add maxItems: 100 and
optionally minItems: 0 or 1 and uniqueItems: true) so the API boundary validates
and caps payload size; locate the AdapterStatusCreateRequest schema and add
these constraints to the conditions array definition referencing
ConditionRequest.
In `@openapi/partner-openapi.yaml`:
- Around line 1-52: The spec lacks a top-level global security declaration, so
add a root-level security entry (e.g., security: - BearerAuth: []) under the
OpenAPI document root (alongside info/tags) to enforce BearerAuth for all
operations by default; update the openapi root where "openapi: 3.0.0" and "info"
are declared and ensure endpoints like operationId getChannels
(/api/hyperfleet/v1/channels) inherit this global security (you can leave or
remove per-operation security entries as desired, but the global security must
be present).
- Around line 1550-1557: The OpenAPI schema defines request-bound arrays without
upper bounds—add appropriate maxItems constraints to the array schemas for
clusterNetwork, serviceNetwork, zones, accelerators, and taints to enforce input
validation; locate the array schema definitions for clusterNetwork (items: $ref
to ClusterNetworkEntry) and serviceNetwork (items: type: string) and add
sensible maxItems values (and mirror the same fixes for the other occurrence
around the second block noted) so both create/patch payloads have explicit upper
bounds and API consumers/servers can validate sizes at boundary checks.
In `@pkg/client/version.go`:
- Around line 22-25: The code returns raw errors from calls like
handleHTTPResponse[partner.Version](resp, http.StatusCreated, "create version")
(and similarly at the other listed sites) which violates the Error Model
Standard; update each return to wrap the downstream error with operation context
(e.g., replace bare "return nil, err" with a wrapped error such as
fmt.Errorf("create version: %w", err) or the project’s preferred wrapping
helper) and do the same for loadPayloadFromFile and all other occurrences
referenced (lines noted in the comment) so callers receive contextualized
errors.
In `@pkg/helper/helper.go`:
- Around line 124-130: The loop that calls h.Client.DeleteVersion currently logs
non-404 failures at Info and continues; change it to treat non-404 as a real
error: after calling h.Client.DeleteVersion(ctx, channelID, *v.Id) check
errors.As(err, &httpErr) and continue only when httpErr.StatusCode ==
http.StatusNotFound, otherwise call logger.Error with a contextual message
(include channel_id and version_id) and return the error (or wrap it) from the
enclosing function (e.g., CleanupTestChannel) instead of continuing; keep the
404 path as the only continue path so partial cleanup surfaces as a failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9285bd4f-e1e2-4c61-b82b-b51a3a41fae3
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sum,!**/go.sumopenapi/specs.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.gitattributesMakefilee2e/channel/crud_lifecycle.goe2e/channel/delete_restrict.goe2e/e2e.goe2e/version/crud_lifecycle.goe2e/version/filtering.goe2e/version/uniqueness.gogo.modhack/tools.goopenapi/core-openapi.yamlopenapi/oapi-codegen-core.yamlopenapi/oapi-codegen-partner.yamlopenapi/partner-openapi.yamlpkg/api/partner/partner.gen.gopkg/client/channel.gopkg/client/client.gopkg/client/cluster.gopkg/client/nodepool.gopkg/client/version.gopkg/helper/helper.gotestdata/payloads/channels/channel-request.jsontestdata/payloads/versions/version-request.json
💤 Files with no reviewable changes (1)
- hack/tools.go
kuudori
force-pushed
the
HYPERFLEET-1088
branch
3 times, most recently
from
200b1be to
e1b35d1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
91-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
cleancurrently deletes tracked client sources.
verify-generatetreatspkg/api/as tracked output, butcleanremoves both generated client trees. Aftermake clean,make build/make teststop working until someone remembers to runmake generatemanually. Either keep committed generated code out ofclean, or restoregenerateas a prerequisite for the standard targets.Suggested minimal fix
clean: ## Remove build artifacts rm -rf $(BIN_DIR) rm -rf $(OUTPUT_DIR) - rm -rf pkg/api/openapi pkg/api/partner rm -f coverage.out coverage.html🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 91 - 95, The clean target currently removes committed generated sources (pkg/api/openapi and pkg/api/partner) causing build/test to fail; either stop deleting those trees from the clean target by removing the "rm -rf pkg/api/openapi pkg/api/partner" lines (and optionally add a separate clean-generated target for untracked cleanup), or make generate a prerequisite for standard targets by adding "generate" as a dependency to build and test (or invoking $(MAKE) generate at the start of their recipes) so verify-generate/tracked outputs are always present; update the Makefile's clean, build, test, verify-generate, and generate references accordingly.
♻️ Duplicate comments (5)
openapi/core-openapi.yaml (2)
1-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet a document-level auth default.
Every operation repeats
BearerAuth, but the spec root has nosecuritydefault. That makes the next added path unauthenticated by default if someone forgets the per-operation stanza.Suggested fix
openapi: 3.0.0 info: title: HyperFleet API version: 1.0.19 +security: + - BearerAuth: [] tags:As per coding guidelines, "Validate changes against HyperFleet architecture standards from the linked architecture repository."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/core-openapi.yaml` around lines 1 - 17, Add a document-level security default so all operations require BearerAuth unless explicitly overridden: update the OpenAPI root to include a top-level "security" array referencing "BearerAuth" and ensure "components.securitySchemes.BearerAuth" is defined and used by existing per-operation entries (e.g., paths currently repeating BearerAuth). This will make the default authentication applied to new paths and prevent accidental unauthenticated endpoints.
1223-1271:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound
AdapterStatusCreateRequest.conditions.This request-body array has no upper bound, so oversized status payloads are still valid per contract. Add
maxItemshere so generated validation can reject pathological requests at the API boundary.Suggested fix
AdapterStatusCreateRequest: type: object required: - adapter - observed_generation - observed_time - conditions properties: @@ conditions: type: array + maxItems: 32 items: $ref: '`#/components/schemas/ConditionRequest`'As per coding guidelines, "Validate input at system boundaries (HTTP handlers, CLI parsers, webhook receivers)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/core-openapi.yaml` around lines 1223 - 1271, The AdapterStatusCreateRequest schema's conditions array is unbounded; add a maxItems constraint to conditions (schema AdapterStatusCreateRequest -> properties.conditions) to cap the number of ConditionRequest entries and reject oversized payloads at the API boundary; choose a reasonable limit (e.g., 50 or as project policy dictates) and update the OpenAPI definition so generated validators enforce conditions.maxItems and include that constraint alongside the existing items: $ref to ConditionRequest.openapi/partner-openapi.yaml (2)
1-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet a document-level auth default here too.
This spec also relies entirely on per-operation
BearerAuth. Without a rootsecurityblock, a future endpoint becomes public if its local stanza is missed.Suggested fix
openapi: 3.0.0 info: title: HyperFleet API version: 1.0.19 +security: + - BearerAuth: [] tags:As per coding guidelines, "Validate changes against HyperFleet architecture standards from the linked architecture repository."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/partner-openapi.yaml` around lines 1 - 22, Add a document-level security default so operations inherit Bearer token auth: define a top-level security entry using the existing BearerAuth scheme (e.g. security: - BearerAuth: []) at the root of the OpenAPI document and ensure components.securitySchemes contains the BearerAuth definition; this prevents accidentally exposing new endpoints that only have per-operation auth and makes all paths require BearerAuth by default while still allowing operation-level overrides.
1547-1557:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCap request-bound arrays in cluster and nodepool specs.
clusterNetwork,serviceNetwork,zones,accelerators, andtaintsare all accepted through create/patch payloads without upper bounds. That leaves oversized requests valid by contract and weakens generated validation.Suggested fix
NetworkingSpec: type: object properties: clusterNetwork: type: array + maxItems: 32 items: $ref: '`#/components/schemas/ClusterNetworkEntry`' serviceNetwork: type: array + maxItems: 32 items: type: string @@ NodePoolPlatform: type: object properties: @@ zones: type: array + maxItems: 16 items: type: string @@ accelerators: type: array + maxItems: 8 items: $ref: '`#/components/schemas/AcceleratorSpec`' @@ taints: type: array + maxItems: 64 items: $ref: '`#/components/schemas/TaintSpec`'As per coding guidelines, "Validate input at system boundaries (HTTP handlers, CLI parsers, webhook receivers)."
Also applies to: 1876-1907
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/partner-openapi.yaml` around lines 1547 - 1557, The OpenAPI schema allows unbounded arrays for NetworkingSpec.clusterNetwork and serviceNetwork (and similarly in the cluster and nodepool specs for properties zones, accelerators, taints), so add reasonable maxItems constraints to each array to enforce request bounds; update the components/schemas entries for NetworkingSpec (clusterNetwork, serviceNetwork) and the relevant ClusterSpec/NodePoolSpec schemas to include maxItems (e.g., maxItems: <appropriate-limit>) and, if needed, minItems or item validation, so generated server/client validation will reject oversized payloads.Makefile (1)
52-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify downloaded specs before replacing pinned inputs.
update-specswrites release assets straight into tracked spec files. A truncated or tampered download here becomes the source for generated clients. Fetch to a temp file and verify an expected checksum (ideally fromopenapi/specs.lock) before moving it into place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 52 - 60, The update-specs Makefile target currently writes release assets directly into openapi/core-openapi.yaml and openapi/partner-openapi.yaml; change it to download each asset to a temporary file first (e.g., /tmp/core-openapi.yaml.tmp and /tmp/partner-openapi.yaml.tmp), compute and verify its checksum against the expected value stored in openapi/specs.lock (use a robust digest like SHA-256), and only atomically move the temp file into place if the checksum matches; on mismatch or failed download, emit an error and exit non‑zero without touching the tracked files; keep the final $(MAKE) generate invocation after both specs are validated and replaced.
🧹 Nitpick comments (1)
pkg/client/channel.go (1)
16-20: 💤 Low valueRedundant error log; inconsistent with sibling methods.
CreateChannellogs atErrorand then returns the wrapped error, whileGetChannel/ListChannels/DeleteChannel/PatchChannelonly wrap-and-return. If the caller also logs the returned error, you get duplicate entries. Pick one convention (wrap-and-return is the prevailing one here) and drop the extra log.♻️ Drop the redundant log
resp, err := c.Partner.PostChannel(ctx, req) if err != nil { - logger.Error("failed to create channel", "name", req.Name, "error", err) return nil, fmt.Errorf("failed to create channel: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client/channel.go` around lines 16 - 20, The CreateChannel method currently logs the error with logger.Error and then returns a wrapped error, causing duplicate logs; remove the logger.Error call in CreateChannel in pkg/client/channel.go so it follows the same wrap-and-return pattern as GetChannel/ListChannels/DeleteChannel/PatchChannel, keeping the existing fmt.Errorf("failed to create channel: %w", err) return so the caller receives the wrapped error but logging is not duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Makefile`:
- Around line 91-95: The clean target currently removes committed generated
sources (pkg/api/openapi and pkg/api/partner) causing build/test to fail; either
stop deleting those trees from the clean target by removing the "rm -rf
pkg/api/openapi pkg/api/partner" lines (and optionally add a separate
clean-generated target for untracked cleanup), or make generate a prerequisite
for standard targets by adding "generate" as a dependency to build and test (or
invoking $(MAKE) generate at the start of their recipes) so
verify-generate/tracked outputs are always present; update the Makefile's clean,
build, test, verify-generate, and generate references accordingly.
---
Duplicate comments:
In `@Makefile`:
- Around line 52-60: The update-specs Makefile target currently writes release
assets directly into openapi/core-openapi.yaml and openapi/partner-openapi.yaml;
change it to download each asset to a temporary file first (e.g.,
/tmp/core-openapi.yaml.tmp and /tmp/partner-openapi.yaml.tmp), compute and
verify its checksum against the expected value stored in openapi/specs.lock (use
a robust digest like SHA-256), and only atomically move the temp file into place
if the checksum matches; on mismatch or failed download, emit an error and exit
non‑zero without touching the tracked files; keep the final $(MAKE) generate
invocation after both specs are validated and replaced.
In `@openapi/core-openapi.yaml`:
- Around line 1-17: Add a document-level security default so all operations
require BearerAuth unless explicitly overridden: update the OpenAPI root to
include a top-level "security" array referencing "BearerAuth" and ensure
"components.securitySchemes.BearerAuth" is defined and used by existing
per-operation entries (e.g., paths currently repeating BearerAuth). This will
make the default authentication applied to new paths and prevent accidental
unauthenticated endpoints.
- Around line 1223-1271: The AdapterStatusCreateRequest schema's conditions
array is unbounded; add a maxItems constraint to conditions (schema
AdapterStatusCreateRequest -> properties.conditions) to cap the number of
ConditionRequest entries and reject oversized payloads at the API boundary;
choose a reasonable limit (e.g., 50 or as project policy dictates) and update
the OpenAPI definition so generated validators enforce conditions.maxItems and
include that constraint alongside the existing items: $ref to ConditionRequest.
In `@openapi/partner-openapi.yaml`:
- Around line 1-22: Add a document-level security default so operations inherit
Bearer token auth: define a top-level security entry using the existing
BearerAuth scheme (e.g. security: - BearerAuth: []) at the root of the OpenAPI
document and ensure components.securitySchemes contains the BearerAuth
definition; this prevents accidentally exposing new endpoints that only have
per-operation auth and makes all paths require BearerAuth by default while still
allowing operation-level overrides.
- Around line 1547-1557: The OpenAPI schema allows unbounded arrays for
NetworkingSpec.clusterNetwork and serviceNetwork (and similarly in the cluster
and nodepool specs for properties zones, accelerators, taints), so add
reasonable maxItems constraints to each array to enforce request bounds; update
the components/schemas entries for NetworkingSpec (clusterNetwork,
serviceNetwork) and the relevant ClusterSpec/NodePoolSpec schemas to include
maxItems (e.g., maxItems: <appropriate-limit>) and, if needed, minItems or item
validation, so generated server/client validation will reject oversized
payloads.
---
Nitpick comments:
In `@pkg/client/channel.go`:
- Around line 16-20: The CreateChannel method currently logs the error with
logger.Error and then returns a wrapped error, causing duplicate logs; remove
the logger.Error call in CreateChannel in pkg/client/channel.go so it follows
the same wrap-and-return pattern as
GetChannel/ListChannels/DeleteChannel/PatchChannel, keeping the existing
fmt.Errorf("failed to create channel: %w", err) return so the caller receives
the wrapped error but logging is not duplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 613c3e1f-a4c7-45c6-bdb9-f831612d8d0c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (25)
.gitattributes.gitignoreMakefilee2e/channel/crud_lifecycle.goe2e/channel/delete_restrict.goe2e/e2e.goe2e/version/crud_lifecycle.goe2e/version/filtering.goe2e/version/uniqueness.gogo.modhack/tools.goopenapi/core-openapi.yamlopenapi/oapi-codegen-core.yamlopenapi/oapi-codegen-partner.yamlopenapi/partner-openapi.yamlpkg/api/openapi/openapi.gen.gopkg/api/partner/partner.gen.gopkg/client/channel.gopkg/client/client.gopkg/client/cluster.gopkg/client/nodepool.gopkg/client/version.gopkg/helper/helper.gotestdata/payloads/channels/channel-request.jsontestdata/payloads/versions/version-request.json
💤 Files with no reviewable changes (2)
- hack/tools.go
- .gitignore
✅ Files skipped from review due to trivial changes (2)
- .gitattributes
- openapi/oapi-codegen-partner.yaml
🚧 Files skipped from review as they are similar to previous changes (14)
- go.mod
- e2e/e2e.go
- e2e/channel/delete_restrict.go
- pkg/client/client.go
- e2e/version/filtering.go
- testdata/payloads/channels/channel-request.json
- pkg/client/cluster.go
- testdata/payloads/versions/version-request.json
- pkg/helper/helper.go
- pkg/client/nodepool.go
- e2e/version/crud_lifecycle.go
- e2e/version/uniqueness.go
- e2e/channel/crud_lifecycle.go
- pkg/client/version.go
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
e2e/channel/delete_restrict.go (1)
27-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the returned objects, not just their
Idfields.
Expect(ch.Id)(Line 29) andExpect(ver.Id)(Line 34) already dereferencech/ver, so a nil object still panics before the guard fires. Add object-level assertions first.Proposed fix
ch, err := h.Client.CreateChannelFromPayload(ctx, h.TestDataPath("payloads/channels/channel-request.json")) Expect(err).NotTo(HaveOccurred(), "failed to create channel") + Expect(ch).NotTo(BeNil(), "created channel should not be nil") Expect(ch.Id).NotTo(BeNil(), "channel ID should not be nil") channelID = *ch.Id ver, err := h.Client.CreateVersionFromPayload(ctx, channelID, h.TestDataPath("payloads/versions/version-request.json")) Expect(err).NotTo(HaveOccurred(), "failed to create version") + Expect(ver).NotTo(BeNil(), "created version should not be nil") Expect(ver.Id).NotTo(BeNil(), "version ID should not be nil") versionID = *ver.IdAs per coding guidelines, "Flag nil/bounds access without guards."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/channel/delete_restrict.go` around lines 27 - 35, The test currently dereferences ch.Id and ver.Id before ensuring ch and ver are non-nil; update the assertions so you guard the returned objects first by asserting Expect(ch).NotTo(BeNil(), "channel object should not be nil") immediately after CreateChannelFromPayload and only then assert Expect(ch.Id).NotTo(BeNil()) and assign channelID; do the same for ver returned by CreateVersionFromPayload (assert Expect(ver).NotTo(BeNil()) before Expect(ver.Id) and assigning versionID). This ensures CreateChannelFromPayload and CreateVersionFromPayload results are checked for nil before accessing their Id fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/version/filtering.go`:
- Around line 71-75: The cleanup registration for the created channel must be
moved to immediately after the channel creation so it runs on all failure paths;
after the code that sets channelID (the channel creation call), call
ginkgo.DeferCleanup with a closure that invokes h.CleanupTestChannel(ctx,
channelID) and logs failures (as currently done) so the cleanup is registered
even if subsequent version creations fail; ensure you use the same
ginkgo.GinkgoWriter.Printf warning message and reference the same channelID
variable and h.CleanupTestChannel function.
In `@pkg/client/channel.go`:
- Line 16: The log call in logger.Info("channel created", "channel_id",
channel.Id, "name", req.Name) is passing channel.Id which is a *string, causing
slog to print the pointer; update the logging to dereference Resource.Id with a
nil check (e.g., compute a local string id variable from channel.Id or use a
helper like idOrEmpty(channel.Id)) and pass that string to logger.Info; apply
the same fix for version logging in pkg/client/version.go where Resource.Id /
version.Id is used.
In `@pkg/client/resource.go`:
- Around line 18-33: Rename the struct field Resource.Id to Resource.ID (use
uppercase "ID" for the acronym) while keeping the json tag
`json:"id,omitempty"`; update all usages that reference the old symbol (e.g.,
channel.Id and version.Id in channel.go and version.go) to use channel.ID and
version.ID respectively so code compiles and follows the acronym casing
guideline; ensure any JSON marshaling/unmarshaling behavior remains unchanged by
preserving the existing tags and run tests/build to verify no remaining
references to `Id`.
- Around line 101-105: The error returned from
loadPayloadFromFile[map[string]any] is being returned bare; change the return to
wrap the original error with context including payloadPath (e.g. using
fmt.Errorf or errors.Wrapf) so callers receive the path and original error
chain; update the block around the loadPayloadFromFile call (the payload, err :=
loadPayloadFromFile[map[string]any](payloadPath) branch) to log as-is with
logger.Error and return a wrapped error (including payloadPath and %w of err).
---
Duplicate comments:
In `@e2e/channel/delete_restrict.go`:
- Around line 27-35: The test currently dereferences ch.Id and ver.Id before
ensuring ch and ver are non-nil; update the assertions so you guard the returned
objects first by asserting Expect(ch).NotTo(BeNil(), "channel object should not
be nil") immediately after CreateChannelFromPayload and only then assert
Expect(ch.Id).NotTo(BeNil()) and assign channelID; do the same for ver returned
by CreateVersionFromPayload (assert Expect(ver).NotTo(BeNil()) before
Expect(ver.Id) and assigning versionID). This ensures CreateChannelFromPayload
and CreateVersionFromPayload results are checked for nil before accessing their
Id fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 753ad1aa-543a-4ec4-a360-02680c7950aa
📒 Files selected for processing (14)
e2e/channel/crud_lifecycle.goe2e/channel/delete_restrict.goe2e/e2e.goe2e/version/crud_lifecycle.goe2e/version/filtering.goe2e/version/uniqueness.gopkg/client/channel.gopkg/client/client.gopkg/client/payload.gopkg/client/resource.gopkg/client/version.gopkg/helper/helper.gotestdata/payloads/channels/channel-request.jsontestdata/payloads/versions/version-request.json
✅ Files skipped from review due to trivial changes (4)
- e2e/e2e.go
- testdata/payloads/versions/version-request.json
- testdata/payloads/channels/channel-request.json
- pkg/client/payload.go
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/version/uniqueness.go
- e2e/channel/crud_lifecycle.go
- e2e/version/crud_lifecycle.go
…c resource client Add generic HTTP resource client (pkg/client/resource.go) for non-codegen API entities. Implement channel and version CRUD wrappers, cleanup helper, and payload template support. Tests cover: CRUD lifecycle, delete-restrict (409 with child versions), spec filtering (is_default, enabled), and name uniqueness per channel.
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/client/version.go (1)
30-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap downstream errors in DeleteVersion and PatchVersion.
Both methods return bare
err(lines 34, 44), which loses operation context and violates ERR-04. CreateVersion demonstrates the correct pattern (line 16).🔧 Proposed fix
func (c *HyperFleetClient) DeleteVersion(ctx context.Context, channelID, versionID string) (*Resource, error) { logger.Info("deleting version", "channel_id", channelID, "version_id", versionID) version, err := c.DeleteResource(ctx, fmt.Sprintf("channels/%s/versions/%s", channelID, versionID)) if err != nil { - return nil, err + return nil, fmt.Errorf("delete version %s in channel %s: %w", versionID, channelID, err) } logger.Info("version deleted", "channel_id", channelID, "version_id", versionID) return version, nil } func (c *HyperFleetClient) PatchVersion(ctx context.Context, channelID, versionID string, req ResourcePatchRequest) (*Resource, error) { logger.Info("patching version", "channel_id", channelID, "version_id", versionID) version, err := c.PatchResource(ctx, fmt.Sprintf("channels/%s/versions/%s", channelID, versionID), req) if err != nil { - return nil, err + return nil, fmt.Errorf("patch version %s in channel %s: %w", versionID, channelID, err) } logger.Info("version patched", "channel_id", channelID, "version_id", versionID, "generation", version.Generation) return version, nil }As per coding guidelines, "Wrap errors per Error Model Standard — no bare return err."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client/version.go` around lines 30 - 48, DeleteVersion and PatchVersion return bare errors which loses context; update both to wrap the downstream errors like CreateVersion does by returning fmt.Errorf with a descriptive message and the original err (e.g., return nil, fmt.Errorf("delete version channel=%s version=%s: %w", channelID, versionID, err)) for DeleteVersion and similarly for PatchVersion (mentioning "patch version" and include generation/context as appropriate) so callers receive contextualized errors; modify the error return sites in DeleteVersion and PatchVersion to use fmt.Errorf with %w to wrap the underlying error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pkg/client/version.go`:
- Around line 30-48: DeleteVersion and PatchVersion return bare errors which
loses context; update both to wrap the downstream errors like CreateVersion does
by returning fmt.Errorf with a descriptive message and the original err (e.g.,
return nil, fmt.Errorf("delete version channel=%s version=%s: %w", channelID,
versionID, err)) for DeleteVersion and similarly for PatchVersion (mentioning
"patch version" and include generation/context as appropriate) so callers
receive contextualized errors; modify the error return sites in DeleteVersion
and PatchVersion to use fmt.Errorf with %w to wrap the underlying error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 261ddb00-af99-43eb-82d3-6855b0ff979e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (14)
e2e/channel/crud_lifecycle.goe2e/channel/delete_restrict.goe2e/e2e.goe2e/version/crud_lifecycle.goe2e/version/filtering.goe2e/version/uniqueness.gogo.modpkg/client/channel.gopkg/client/client.gopkg/client/resource.gopkg/client/version.gopkg/helper/helper.gotestdata/payloads/channels/channel-request.jsontestdata/payloads/versions/version-request.json
✅ Files skipped from review due to trivial changes (2)
- testdata/payloads/channels/channel-request.json
- testdata/payloads/versions/version-request.json
🚧 Files skipped from review as they are similar to previous changes (9)
- e2e/e2e.go
- e2e/version/uniqueness.go
- pkg/helper/helper.go
- e2e/channel/delete_restrict.go
- pkg/client/resource.go
- e2e/channel/crud_lifecycle.go
- pkg/client/client.go
- pkg/client/channel.go
- e2e/version/crud_lifecycle.go
Contributor
Author
|
/retest |
Contributor
|
Review — nit (JIRA) The PR body still contains the template placeholder Suggestion: Update the Summary section from |
rafabene
reviewed
Jun 3, 2026
| github.com/pelletier/go-toml/v2 v2.1.0 // indirect | ||
| github.com/sagikazarmark/locafero v0.4.0 // indirect | ||
| github.com/sagikazarmark/slog-shim v0.1.0 // indirect | ||
| github.com/samber/lo v1.53.0 // indirect |
Contributor
There was a problem hiding this comment.
nit: samber/lo is marked // indirect but is directly imported in pkg/client/channel.go and pkg/client/version.go. Run go mod tidy to fix.
Also, this dependency is added for just two lo.FromPtr() calls in log messages. Consider a local helper or inline dereference to avoid the extra dependency:
// In the log lines, channel.Id is already nil-checked by Expect above
logger.Info("channel created", "channel_id", *channel.Id, "name", req.Name)
Contributor
|
/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 |
rafabene
reviewed
Jun 3, 2026
| ginkgo.It("should delete channel", func(ctx context.Context) { | ||
| ginkgo.By("deleting channel") | ||
| deleted, err := h.Client.DeleteChannel(ctx, channelID) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to delete channel") |
Contributor
There was a problem hiding this comment.
nit: The PATCH test sends both is_default and enabled_regex but only verifies is_default after the GET. The version CRUD test verifies all patched fields — this one should too for consistency.
Suggested change
| Expect(err).NotTo(HaveOccurred(), "failed to delete channel") | |
| Expect(fetched.Spec["is_default"]).To(Equal(true)) | |
| Expect(fetched.Spec["enabled_regex"]).To(Equal(".*")) |
Contributor
Author
|
/retest |
openshift-merge-bot
Bot
merged commit 502cc6d
into
openshift-hyperfleet:main
7 checks passed
|
@kuudori: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace Go module spec consumption with vendored release artifacts. Generate typed partner client (Channel, Version) alongside core client. Split version tests into separate e2e/version/ suite.
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)