HYPERFLEET-1202 - refactor: derive E2E HTTP client from api-spec-template#138
Draft
rafabene wants to merge 1 commit into
Draft
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds generated Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/channel/crud_lifecycle.go (1)
75-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert
EnabledRegextoo. The GET after PATCH only checksIsDefault; add an assertion forEnabledRegexso a regression in the patched regex can’t slip through.🤖 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/crud_lifecycle.go` around lines 75 - 90, The PATCH/GET lifecycle check in the channel CRUD test only verifies IsDefault, so a regression in EnabledRegex could be missed. Update the verification after h.Client.GetChannel in the crud_lifecycle.go test to also assert fetched.Spec.EnabledRegex matches the patched enabledRegex value, alongside the existing IsDefault check, so both fields set by PatchChannel are validated.
🧹 Nitpick comments (4)
pkg/client/channel.go (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap propagated channel errors.
These bare returns drop operation-specific context. Keep the generated HTTP status handling, but wrap the error before returning.
Wrap response and payload errors
channel, err := handleHTTPResponse[openapi.Channel](resp, http.StatusCreated, "create channel") if err != nil { - return nil, err + return nil, fmt.Errorf("create channel response: %w", err) } @@ channel, err := handleHTTPResponse[openapi.Channel](resp, http.StatusAccepted, "delete channel") if err != nil { - return nil, err + return nil, fmt.Errorf("delete channel %q response: %w", channelID, err) } @@ channel, err := handleHTTPResponse[openapi.Channel](resp, http.StatusOK, "patch channel") if err != nil { - return nil, err + return nil, fmt.Errorf("patch channel %q response: %w", channelID, err) } @@ if err != nil { logger.Error("failed to load payload", "payload_path", payloadPath, "error", err) - return nil, err + return nil, fmt.Errorf("load channel payload %q: %w", payloadPath, err) }As per path instructions, “Wrap errors per Error Model Standard — no bare return err.”
Also applies to: 59-61, 76-78, 88-91
🤖 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 22 - 24, The create channel flow in channel.go is returning a bare error from handleHTTPResponse without operation context. Update the affected return paths in the channel-related functions (including the create/retrieve/update/delete helpers) so they wrap the propagated error with the operation name before returning, while keeping the existing HTTP status checks and generated response handling intact.Source: Path instructions
pkg/client/wifconfig.go (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap propagated WifConfig errors.
The direct returns drop request context from HTTP decode/status failures and payload loading failures.
Wrap response and payload errors
wifConfig, err := handleHTTPResponse[openapi.WifConfig](resp, http.StatusCreated, "create wifconfig") if err != nil { - return nil, err + return nil, fmt.Errorf("create wifconfig %q response: %w", req.Name, err) } @@ wifConfig, err := handleHTTPResponse[openapi.WifConfig](resp, http.StatusAccepted, "delete wifconfig") if err != nil { - return nil, err + return nil, fmt.Errorf("delete wifconfig %q response: %w", wifConfigID, err) } @@ wifConfig, err := handleHTTPResponse[openapi.WifConfig](resp, http.StatusOK, "patch wifconfig") if err != nil { - return nil, err + return nil, fmt.Errorf("patch wifconfig %q response: %w", wifConfigID, err) } @@ if err != nil { logger.Error("failed to load payload", "payload_path", payloadPath, "error", err) - return nil, err + return nil, fmt.Errorf("load wifconfig payload %q: %w", payloadPath, err) }As per path instructions, “Wrap errors per Error Model Standard — no bare return err.”
Also applies to: 59-61, 76-78, 88-91
🤖 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/wifconfig.go` around lines 22 - 24, Wrap the propagated WifConfig failures so they preserve request context instead of returning the raw error. In the WifConfig client flow, update the direct error returns around handleHTTPResponse, payload loading, and related call sites in the WifConfig methods to use the project’s Error Model Standard with contextual wrapping; use the surrounding method names and request intent (for example the create/update/read WifConfig paths) to identify each bare return err and replace them consistently.Source: Path instructions
pkg/client/version.go (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap propagated version errors.
Bare returns lose the channel/version identifiers that make E2E failures diagnosable.
Wrap response and payload errors
version, err := handleHTTPResponse[openapi.Version](resp, http.StatusCreated, "create version") if err != nil { - return nil, err + return nil, fmt.Errorf("create version %q in channel %s response: %w", req.Name, channelID, err) } @@ version, err := handleHTTPResponse[openapi.Version](resp, http.StatusAccepted, "delete version") if err != nil { - return nil, err + return nil, fmt.Errorf("delete version %q in channel %s response: %w", versionID, channelID, err) } @@ version, err := handleHTTPResponse[openapi.Version](resp, http.StatusOK, "patch version") if err != nil { - return nil, err + return nil, fmt.Errorf("patch version %q in channel %s response: %w", versionID, channelID, err) } @@ if err != nil { logger.Error("failed to load payload", "channel_id", channelID, "payload_path", payloadPath, "error", err) - return nil, err + return nil, fmt.Errorf("load version payload %q for channel %s: %w", payloadPath, channelID, err) }As per path instructions, “Wrap errors per Error Model Standard — no bare return err.”
Also applies to: 59-61, 76-78, 88-91
🤖 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 22 - 24, The version-related error paths in version.go are returning raw errors, which drops the channel/version context needed for diagnosis. Update the response and payload handling in the version creation flow and the other mentioned version helpers so that errors are wrapped before returning, using the existing function names like handleHTTPResponse and the version create/update logic to add channel/version identifiers and operation context. Keep the same control flow, but replace bare err propagation with wrapped errors that preserve the underlying cause and relevant version metadata.Source: Path instructions
e2e/nodepool/concurrent_creation.go (1)
143-143: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePreallocate
adapterMapwith known capacity.
len(statuses.Items)is known here (same pattern already applied correctly ine2e/cluster/stuck_deletion.go). Same gap repeats ine2e/nodepool/creation.go:59.As per path instructions, "preallocate slices/maps when size is known."
Proposed fix
- adapterMap := make(map[string]core.AdapterStatus) + adapterMap := make(map[string]core.AdapterStatus, len(statuses.Items))🤖 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/nodepool/concurrent_creation.go` at line 143, The adapterMap initialization in the nodepool concurrent creation flow should be preallocated because the number of status items is already known. Update the map creation in the logic that builds adapterMap from statuses.Items to use the existing item count as capacity, following the same pattern used in the stuck deletion path and the creation flow. This change should be applied wherever adapterMap is constructed in this routine so the map starts with the expected size instead of growing dynamically.Source: Path instructions
🤖 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 `@Makefile`:
- Around line 46-63: The Makefile’s generate target currently downloads mutable
OpenAPI artifacts using API_SPEC_VERSION and API_SPEC_TEMPLATE_VERSION defaults
of latest, with no integrity verification. Update the generate flow to use
immutable release versions instead of latest, add SHA-256 checksum validation
for the downloaded files, and make the curl calls fail on HTTP errors so the
generate step cannot proceed with unexpected or tampered specs; apply this in
the generate target where the openapi/openapi.yaml and openapi/core-openapi.yaml
downloads are performed.
---
Outside diff comments:
In `@e2e/channel/crud_lifecycle.go`:
- Around line 75-90: The PATCH/GET lifecycle check in the channel CRUD test only
verifies IsDefault, so a regression in EnabledRegex could be missed. Update the
verification after h.Client.GetChannel in the crud_lifecycle.go test to also
assert fetched.Spec.EnabledRegex matches the patched enabledRegex value,
alongside the existing IsDefault check, so both fields set by PatchChannel are
validated.
---
Nitpick comments:
In `@e2e/nodepool/concurrent_creation.go`:
- Line 143: The adapterMap initialization in the nodepool concurrent creation
flow should be preallocated because the number of status items is already known.
Update the map creation in the logic that builds adapterMap from statuses.Items
to use the existing item count as capacity, following the same pattern used in
the stuck deletion path and the creation flow. This change should be applied
wherever adapterMap is constructed in this routine so the map starts with the
expected size instead of growing dynamically.
In `@pkg/client/channel.go`:
- Around line 22-24: The create channel flow in channel.go is returning a bare
error from handleHTTPResponse without operation context. Update the affected
return paths in the channel-related functions (including the
create/retrieve/update/delete helpers) so they wrap the propagated error with
the operation name before returning, while keeping the existing HTTP status
checks and generated response handling intact.
In `@pkg/client/version.go`:
- Around line 22-24: The version-related error paths in version.go are returning
raw errors, which drops the channel/version context needed for diagnosis. Update
the response and payload handling in the version creation flow and the other
mentioned version helpers so that errors are wrapped before returning, using the
existing function names like handleHTTPResponse and the version create/update
logic to add channel/version identifiers and operation context. Keep the same
control flow, but replace bare err propagation with wrapped errors that preserve
the underlying cause and relevant version metadata.
In `@pkg/client/wifconfig.go`:
- Around line 22-24: Wrap the propagated WifConfig failures so they preserve
request context instead of returning the raw error. In the WifConfig client
flow, update the direct error returns around handleHTTPResponse, payload
loading, and related call sites in the WifConfig methods to use the project’s
Error Model Standard with contextual wrapping; use the surrounding method names
and request intent (for example the create/update/read WifConfig paths) to
identify each bare return err and replace them consistently.
🪄 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: b79c90ef-161e-472a-80ff-32db1e885b35
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (45)
.gitignoreMakefilee2e/adapter/adapter_failover.goe2e/adapter/adapter_with_maestro.goe2e/adapter/maestro_unavailability.goe2e/channel/crud_lifecycle.goe2e/cluster/adapter_failure.goe2e/cluster/concurrent_creation.goe2e/cluster/crash_recovery.goe2e/cluster/creation.goe2e/cluster/delete.goe2e/cluster/delete_edge_cases.goe2e/cluster/delete_external.goe2e/cluster/force_delete.goe2e/cluster/lifecycle_smoke.goe2e/cluster/perf_cascade_delete_latency.goe2e/cluster/perf_create_latency.goe2e/cluster/perf_delete_latency.goe2e/cluster/perf_read_entity_size_latency.goe2e/cluster/perf_update_latency.goe2e/cluster/stuck_deletion.goe2e/cluster/update.goe2e/cluster/update_edge_cases.goe2e/nodepool/concurrent_creation.goe2e/nodepool/creation.goe2e/nodepool/delete.goe2e/nodepool/delete_edge_cases.goe2e/nodepool/lifecycle_smoke.goe2e/nodepool/perf_create_latency.goe2e/nodepool/perf_delete_latency.goe2e/nodepool/update.goe2e/nodepool/update_edge_cases.goe2e/version/crud_lifecycle.goe2e/wifconfig/crud_lifecycle.gogo.modhack/tools.goopenapi/oapi-codegen-core.yamlpkg/client/channel.gopkg/client/cluster.gopkg/client/nodepool.gopkg/client/version.gopkg/client/wifconfig.gopkg/helper/matchers.gopkg/helper/pollers.gopkg/helper/validation.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (2)
- hack/tools.go
- go.mod
rafabene
force-pushed
the
HYPERFLEET-1202-refactor-e2e-http-client
branch
from
1b11559 to
815f79e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Makefile (1)
46-63: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winChecksum verification still missing (CWE-494).
Version pinning +
-foncurl(Lines 47-48, 56-60) fixes thelatest-tag drift from the prior review, but the downloaded spec YAMLs still have zero integrity verification. A compromised or re-tagged GitHub release forhyperfleet-api-spec-template/hyperfleet-api-specat these pinned tags would silently alter generated client code (CWE-494: Download of Code Without Integrity Check). "Immutable tags" is not a substitute for checksum pinning — GitHub release assets under a given tag can still be replaced by repo maintainers/compromised tokens.As per path instructions,
**/Makefilerequires: "No curl-pipe-bash for tool installation (use checksummed downloads)".🔒 Add checksum pinning
API_SPEC_VERSION ?= v1.0.25 API_SPEC_TEMPLATE_VERSION ?= v1.0.26 +API_SPEC_SHA256 ?= <pin-me> +API_SPEC_TEMPLATE_SHA256 ?= <pin-me> .PHONY: generate generate: $(OAPI_CODEGEN) ## Generate API client code from OpenAPI schema rm -f pkg/api/openapi/openapi.gen.go pkg/api/core/core.gen.go mkdir -p pkg/api/openapi pkg/api/core openapi `@rm` -f openapi/openapi.yaml openapi/core-openapi.yaml `@echo` "Downloading template spec ($(API_SPEC_TEMPLATE_VERSION))..." `@curl` -sfL -o openapi/openapi.yaml \ "https://github.com/openshift-hyperfleet/hyperfleet-api-spec-template/releases/download/$(API_SPEC_TEMPLATE_VERSION)/template-openapi.yaml" + `@echo` "$(API_SPEC_TEMPLATE_SHA256) openapi/openapi.yaml" | sha256sum -c - `@echo` "Downloading core spec ($(API_SPEC_VERSION))..." `@curl` -sfL -o openapi/core-openapi.yaml \ "https://github.com/openshift-hyperfleet/hyperfleet-api-spec/releases/download/$(API_SPEC_VERSION)/core-openapi.yaml" + `@echo` "$(API_SPEC_SHA256) openapi/core-openapi.yaml" | sha256sum -c -🤖 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 46 - 63, The generate target in Makefile still downloads OpenAPI spec assets without integrity checks, so add checksum verification for the files fetched by the curl commands in generate. Update the workflow around API_SPEC_VERSION, API_SPEC_TEMPLATE_VERSION, and the downloads for openapi/openapi.yaml and openapi/core-openapi.yaml to validate each asset against a pinned checksum before running OAPI_CODEGEN. Keep the fix localized to the generate rule and make it fail fast if the downloaded content does not match the expected digest.Source: Path instructions
🤖 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 `@Makefile`:
- Around line 46-63: The generate target in Makefile still downloads OpenAPI
spec assets without integrity checks, so add checksum verification for the files
fetched by the curl commands in generate. Update the workflow around
API_SPEC_VERSION, API_SPEC_TEMPLATE_VERSION, and the downloads for
openapi/openapi.yaml and openapi/core-openapi.yaml to validate each asset
against a pinned checksum before running OAPI_CODEGEN. Keep the fix localized to
the generate rule and make it fail fast if the downloaded content does not match
the expected digest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e912c016-4160-4cff-93f2-aa51266352bc
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (45)
.gitignoreMakefilee2e/adapter/adapter_failover.goe2e/adapter/adapter_with_maestro.goe2e/adapter/maestro_unavailability.goe2e/channel/crud_lifecycle.goe2e/cluster/adapter_failure.goe2e/cluster/concurrent_creation.goe2e/cluster/crash_recovery.goe2e/cluster/creation.goe2e/cluster/delete.goe2e/cluster/delete_edge_cases.goe2e/cluster/delete_external.goe2e/cluster/force_delete.goe2e/cluster/lifecycle_smoke.goe2e/cluster/perf_cascade_delete_latency.goe2e/cluster/perf_create_latency.goe2e/cluster/perf_delete_latency.goe2e/cluster/perf_read_entity_size_latency.goe2e/cluster/perf_update_latency.goe2e/cluster/stuck_deletion.goe2e/cluster/update.goe2e/cluster/update_edge_cases.goe2e/nodepool/concurrent_creation.goe2e/nodepool/creation.goe2e/nodepool/delete.goe2e/nodepool/delete_edge_cases.goe2e/nodepool/lifecycle_smoke.goe2e/nodepool/perf_create_latency.goe2e/nodepool/perf_delete_latency.goe2e/nodepool/update.goe2e/nodepool/update_edge_cases.goe2e/version/crud_lifecycle.goe2e/wifconfig/crud_lifecycle.gogo.modhack/tools.goopenapi/oapi-codegen-core.yamlpkg/client/channel.gopkg/client/cluster.gopkg/client/nodepool.gopkg/client/version.gopkg/client/wifconfig.gopkg/helper/matchers.gopkg/helper/pollers.gopkg/helper/validation.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (2)
- hack/tools.go
- go.mod
✅ Files skipped from review due to trivial changes (6)
- e2e/nodepool/perf_delete_latency.go
- e2e/nodepool/delete_edge_cases.go
- e2e/cluster/force_delete.go
- openapi/oapi-codegen-core.yaml
- e2e/nodepool/update_edge_cases.go
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (36)
- e2e/cluster/lifecycle_smoke.go
- e2e/nodepool/perf_create_latency.go
- e2e/cluster/update.go
- e2e/cluster/perf_cascade_delete_latency.go
- e2e/cluster/delete_edge_cases.go
- e2e/cluster/perf_delete_latency.go
- e2e/cluster/perf_create_latency.go
- e2e/cluster/perf_read_entity_size_latency.go
- e2e/cluster/delete.go
- e2e/cluster/perf_update_latency.go
- e2e/cluster/delete_external.go
- e2e/cluster/update_edge_cases.go
- e2e/channel/crud_lifecycle.go
- e2e/version/crud_lifecycle.go
- pkg/helper/validation.go
- e2e/nodepool/concurrent_creation.go
- e2e/cluster/concurrent_creation.go
- e2e/nodepool/creation.go
- e2e/nodepool/delete.go
- e2e/cluster/adapter_failure.go
- e2e/nodepool/update.go
- pkg/helper/pollers.go
- e2e/cluster/crash_recovery.go
- e2e/cluster/stuck_deletion.go
- e2e/wifconfig/crud_lifecycle.go
- e2e/adapter/adapter_failover.go
- pkg/client/nodepool.go
- e2e/adapter/maestro_unavailability.go
- pkg/client/version.go
- e2e/adapter/adapter_with_maestro.go
- e2e/cluster/creation.go
- pkg/client/wifconfig.go
- pkg/helper/matchers.go
- pkg/client/cluster.go
- e2e/nodepool/lifecycle_smoke.go
- pkg/client/channel.go
rafabene
force-pushed
the
HYPERFLEET-1202-refactor-e2e-http-client
branch
from
815f79e to
e27beda
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
46-63: 🔒 Security & Privacy | 🔵 TrivialEnable GitHub Immutable Releases on the spec repos. It locks release assets and tags after publish, closing the remaining CWE-494/CWE-829 supply-chain window without adding checksum handling here.
🤖 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 46 - 63, Update the OpenAPI spec download flow in the generate target to rely on GitHub Immutable Releases for both spec repositories, since the current curl-based pulls from the versioned release assets still depend on mutable release contents. Keep the existing generate workflow and symbols like API_SPEC_VERSION, API_SPEC_TEMPLATE_VERSION, and generate, but adjust the source/versioning approach so the downloaded assets are from immutable releases after publish.
🤖 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.
Nitpick comments:
In `@Makefile`:
- Around line 46-63: Update the OpenAPI spec download flow in the generate
target to rely on GitHub Immutable Releases for both spec repositories, since
the current curl-based pulls from the versioned release assets still depend on
mutable release contents. Keep the existing generate workflow and symbols like
API_SPEC_VERSION, API_SPEC_TEMPLATE_VERSION, and generate, but adjust the
source/versioning approach so the downloaded assets are from immutable releases
after publish.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1ace306c-74d3-4b24-95b4-8d41d9ad44ee
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (46)
.gitignoreMakefilee2e/adapter/adapter_failover.goe2e/adapter/adapter_with_maestro.goe2e/adapter/maestro_unavailability.goe2e/channel/crud_lifecycle.goe2e/cluster/adapter_failure.goe2e/cluster/concurrent_creation.goe2e/cluster/crash_recovery.goe2e/cluster/creation.goe2e/cluster/delete.goe2e/cluster/delete_edge_cases.goe2e/cluster/delete_external.goe2e/cluster/force_delete.goe2e/cluster/lifecycle_smoke.goe2e/cluster/perf_cascade_delete_latency.goe2e/cluster/perf_create_latency.goe2e/cluster/perf_delete_latency.goe2e/cluster/perf_read_entity_size_latency.goe2e/cluster/perf_update_latency.goe2e/cluster/stuck_deletion.goe2e/cluster/update.goe2e/cluster/update_edge_cases.goe2e/nodepool/concurrent_creation.goe2e/nodepool/creation.goe2e/nodepool/delete.goe2e/nodepool/delete_edge_cases.goe2e/nodepool/lifecycle_smoke.goe2e/nodepool/perf_create_latency.goe2e/nodepool/perf_delete_latency.goe2e/nodepool/update.goe2e/nodepool/update_edge_cases.goe2e/version/crud_lifecycle.goe2e/wifconfig/crud_lifecycle.gogo.modhack/tools.goopenapi/oapi-codegen-core.yamlpkg/client/channel.gopkg/client/cluster.gopkg/client/nodepool.gopkg/client/version.gopkg/client/wifconfig.gopkg/helper/matchers.gopkg/helper/pollers.gopkg/helper/validation.gotestdata/payloads/nodepools/nodepool-patch.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (2)
- go.mod
- hack/tools.go
✅ Files skipped from review due to trivial changes (4)
- e2e/cluster/perf_cascade_delete_latency.go
- e2e/cluster/perf_read_entity_size_latency.go
- e2e/cluster/perf_update_latency.go
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (38)
- e2e/nodepool/perf_delete_latency.go
- e2e/nodepool/update_edge_cases.go
- e2e/cluster/perf_create_latency.go
- e2e/cluster/update.go
- e2e/nodepool/perf_create_latency.go
- e2e/cluster/perf_delete_latency.go
- e2e/cluster/lifecycle_smoke.go
- e2e/nodepool/delete.go
- e2e/nodepool/lifecycle_smoke.go
- e2e/cluster/delete_edge_cases.go
- pkg/helper/validation.go
- openapi/oapi-codegen-core.yaml
- e2e/cluster/delete_external.go
- e2e/nodepool/update.go
- e2e/adapter/maestro_unavailability.go
- pkg/helper/pollers.go
- e2e/cluster/crash_recovery.go
- e2e/cluster/force_delete.go
- e2e/wifconfig/crud_lifecycle.go
- e2e/channel/crud_lifecycle.go
- e2e/cluster/delete.go
- pkg/client/cluster.go
- e2e/cluster/concurrent_creation.go
- pkg/client/nodepool.go
- e2e/version/crud_lifecycle.go
- e2e/cluster/update_edge_cases.go
- e2e/cluster/adapter_failure.go
- e2e/nodepool/creation.go
- e2e/nodepool/delete_edge_cases.go
- e2e/nodepool/concurrent_creation.go
- pkg/helper/matchers.go
- pkg/client/channel.go
- e2e/adapter/adapter_failover.go
- e2e/cluster/creation.go
- e2e/cluster/stuck_deletion.go
- pkg/client/version.go
- e2e/adapter/adapter_with_maestro.go
- pkg/client/wifconfig.go
Member
Author
|
/retest |
Member
Author
|
The only remaining E2E failure (Maestro transport test) is caused by the template spec missing the |
rafabene
force-pushed
the
HYPERFLEET-1202-refactor-e2e-http-client
branch
from
e27beda to
19befbc
Compare
1 task
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/adapter/adapter_with_maestro.go`:
- Around line 197-201: The failure message for the ConfigMap assertion is stale
and still refers to .platformType "gcp" even though the surrounding verification
in the adapter_with_maestro test uses .platformType "template". Update the
g.Expect(..., ...) message in the relevant test block so it matches the actual
Go template condition being verified, keeping the context tied to the ConfigMap
and platform_tier assertion.
🪄 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: bfa80280-7696-4734-8edc-3f97a9d2fe43
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (50)
.gitignoreMakefilee2e/adapter/adapter_failover.goe2e/adapter/adapter_with_maestro.goe2e/adapter/maestro_unavailability.goe2e/channel/crud_lifecycle.goe2e/cluster/adapter_failure.goe2e/cluster/concurrent_creation.goe2e/cluster/crash_recovery.goe2e/cluster/creation.goe2e/cluster/delete.goe2e/cluster/delete_edge_cases.goe2e/cluster/delete_external.goe2e/cluster/force_delete.goe2e/cluster/lifecycle_smoke.goe2e/cluster/perf_cascade_delete_latency.goe2e/cluster/perf_create_latency.goe2e/cluster/perf_delete_latency.goe2e/cluster/perf_read_entity_size_latency.goe2e/cluster/perf_update_latency.goe2e/cluster/stuck_deletion.goe2e/cluster/update.goe2e/cluster/update_edge_cases.goe2e/nodepool/concurrent_creation.goe2e/nodepool/creation.goe2e/nodepool/delete.goe2e/nodepool/delete_edge_cases.goe2e/nodepool/lifecycle_smoke.goe2e/nodepool/perf_create_latency.goe2e/nodepool/perf_delete_latency.goe2e/nodepool/update.goe2e/nodepool/update_edge_cases.goe2e/version/crud_lifecycle.goe2e/wifconfig/crud_lifecycle.gogo.modhack/tools.goopenapi/oapi-codegen-core.yamlpkg/client/channel.gopkg/client/cluster.gopkg/client/nodepool.gopkg/client/version.gopkg/client/wifconfig.gopkg/helper/matchers.gopkg/helper/pollers.gopkg/helper/validation.gotestdata/payloads/clusters/cluster-patch.jsontestdata/payloads/clusters/cluster-request-large.jsontestdata/payloads/clusters/cluster-request-small.jsontestdata/payloads/clusters/cluster-request.jsontestdata/payloads/nodepools/nodepool-patch.json
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (2)
- go.mod
- hack/tools.go
✅ Files skipped from review due to trivial changes (4)
- e2e/cluster/force_delete.go
- openapi/oapi-codegen-core.yaml
- e2e/cluster/update.go
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (38)
- e2e/nodepool/perf_create_latency.go
- e2e/cluster/perf_update_latency.go
- e2e/cluster/lifecycle_smoke.go
- e2e/cluster/perf_read_entity_size_latency.go
- e2e/nodepool/lifecycle_smoke.go
- e2e/nodepool/perf_delete_latency.go
- e2e/nodepool/update_edge_cases.go
- e2e/cluster/perf_cascade_delete_latency.go
- e2e/cluster/perf_delete_latency.go
- e2e/version/crud_lifecycle.go
- e2e/cluster/perf_create_latency.go
- e2e/nodepool/update.go
- e2e/cluster/delete_external.go
- pkg/helper/pollers.go
- e2e/nodepool/delete_edge_cases.go
- testdata/payloads/nodepools/nodepool-patch.json
- e2e/wifconfig/crud_lifecycle.go
- pkg/helper/validation.go
- e2e/cluster/crash_recovery.go
- e2e/adapter/maestro_unavailability.go
- e2e/channel/crud_lifecycle.go
- pkg/client/cluster.go
- e2e/cluster/creation.go
- e2e/cluster/adapter_failure.go
- e2e/cluster/delete_edge_cases.go
- e2e/cluster/concurrent_creation.go
- e2e/adapter/adapter_failover.go
- pkg/helper/matchers.go
- e2e/cluster/update_edge_cases.go
- e2e/nodepool/creation.go
- e2e/nodepool/concurrent_creation.go
- e2e/cluster/stuck_deletion.go
- e2e/cluster/delete.go
- pkg/client/nodepool.go
- e2e/nodepool/delete.go
- pkg/client/channel.go
- pkg/client/version.go
- pkg/client/wifconfig.go
|
@rafabene: 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. |
…late Refactor E2E tests to generate the HTTP client from api-spec-template (partner contract) instead of api-spec (core contract). Both specs are downloaded from GitHub releases via curl, keeping spec repos language-agnostic with no Go module dependency. - Template spec generates typed client for partner entities (Clusters, NodePools, Channels, Versions, WifConfigs) into pkg/api/openapi/ - Core spec generates internal types (AdapterStatus, AdapterCondition, ForceDeleteRequest) into pkg/api/core/ - Channel, Version, WifConfig client methods now use generated typed models instead of generic Resource with map[string]any - Cluster/NodePool status and force-delete endpoints use doJSON since they are internal-only and not in the template spec - E2E tests updated to use openapi.* for partner types and core.* for internal types - Spec versions pinned to immutable tags with curl --fail - Cluster payloads updated to use platform type template
rafabene
force-pushed
the
HYPERFLEET-1202-refactor-e2e-http-client
branch
from
19befbc to
5a6ae74
Compare
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
1 participant
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.
Summary
pkg/api/openapi/pkg/api/core/Resourcewithmap[string]anydoJSON(internal-only, not in template spec)Test plan
make checkpasses (generate, fmt-check, vet, lint, test)make buildcompiles successfully