HYPERFLEET-769 - fix: align golangci.yml with architecture standard and resolve all lint violations#83
Open
rafabene wants to merge 3 commits intoopenshift-hyperfleet:mainfrom
Open
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 |
|
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:
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 (2)
WalkthroughThis PR renames many internal Go packages to remove underscores (config_loader→configloader, hyperfleet_api→hyperfleetapi, k8s_client→k8sclient, maestro_client→maestroclient, transport_client→transportclient) and updates all imports, type usages, tests, and integration suites accordingly. It tightens .golangci.yml lint rules, normalizes file-read paths with filepath.Clean, converts several if/else chains to switch statements, replaces math/rand jitter with crypto/rand (with fallback), adjusts log/error spelling from "cancelled"→"canceled", improves HTTP Body.Close() error handling, and makes minor formatting/refactor changes across many files. Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 4
🧹 Nitpick comments (4)
test/integration/testutil/mock_api_server.go (1)
82-85: Consider usingerrors.Isfor EOF check.String comparison is fragile if errors get wrapped. Use idiomatic Go error checking:
+import "errors" +import "io" + n, readErr := r.Body.Read(buf) -if readErr != nil && readErr.Error() != "EOF" { +if readErr != nil && !errors.Is(readErr, io.EOF) { t.Logf("Warning: error reading request body: %v", readErr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/testutil/mock_api_server.go` around lines 82 - 85, Replace the fragile string comparison for EOF when reading the request body: instead of checking readErr.Error() != "EOF", use errors.Is(readErr, io.EOF) to detect EOF; update the condition around r.Body.Read(buf) and t.Logf to only log non-EOF errors, and add the necessary imports (errors and io) if they’re not already present so the check uses errors.Is(readErr, io.EOF) with the existing variables readErr, r.Body.Read, buf, and t.Logf.internal/config_loader/struct_validator.go (1)
68-73: Fail fast on custom validator registration errors.These calls run once during singleton initialization with hard-coded tags. Silencing the returned error means a future typo leaves the validator partially configured until runtime validation hits the missing rule.
Possible cleanup
- // Register custom field-level validations - // these validations are known-good, errors would only occur on invalid config - //nolint:errcheck - _ = structValidator.RegisterValidation("resourcename", validateResourceName) - //nolint:errcheck - _ = structValidator.RegisterValidation("validoperator", validateOperator) + // Register custom field-level validations + mustRegisterValidation := func(tag string, fn validator.Func) { + if err := structValidator.RegisterValidation(tag, fn); err != nil { + panic(fmt.Errorf("register %q validator: %w", tag, err)) + } + } + mustRegisterValidation("resourcename", validateResourceName) + mustRegisterValidation("validoperator", validateOperator)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config_loader/struct_validator.go` around lines 68 - 73, The RegisterValidation calls for custom validators ("resourcename" and "validoperator") are currently ignoring returned errors; update the initialization in struct_validator.go to fail fast by checking the returned error from structValidator.RegisterValidation for "resourcename" (validateResourceName) and "validoperator" (validateOperator) and handling it (e.g., log.Fatal / panic or return the error from the init function) so typos or registration failures surface at startup rather than at runtime validation.test/integration/executor/executor_k8s_integration_test.go (1)
285-287: Cover theclusterIDpayload rename with an assertion.This helper now changes the outgoing status JSON key from
clusterIdtoclusterID, but the integration checks still only inspectconditions. Please assert the recorded payload field as well so this contract change stays intentional and covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/executor/executor_k8s_integration_test.go` around lines 285 - 287, The test now needs an explicit assertion that the outgoing status JSON uses the renamed key "clusterID" (not "clusterId"); update the integration test in executor_k8s_integration_test.go to inspect the recorded status payload (the same object you currently check for "conditions") and assert that payload["clusterID"] equals the expected cluster ID value (the template value used in the helper). Locate the place where you validate "conditions" and add a similar assertion for "clusterID" so the contract change is covered by the test.test/integration/maestro_client/client_integration_test.go (1)
223-225: Prefer explicit map extraction over chained type assertions.This line can panic if the fixture shape changes and the
nolint:lllhides the readability issue. Use checked extraction so failures are explicit.Proposed refactor
- //nolint:lll - configMapManifest["metadata"].(map[string]interface{})["annotations"].(map[string]interface{})[constants.AnnotationGeneration] = "2" + metadata, ok := configMapManifest["metadata"].(map[string]interface{}) + require.True(t, ok, "metadata should be a map") + annotations, ok := metadata["annotations"].(map[string]interface{}) + require.True(t, ok, "metadata.annotations should be a map") + annotations[constants.AnnotationGeneration] = "2"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/maestro_client/client_integration_test.go` around lines 223 - 225, The chained type assertions on configMapManifest (e.g., configMapManifest["metadata"].(map[string]interface{})["annotations"].(map[string]interface{})[constants.AnnotationGeneration] = "2" and configMapManifest["data"].(map[string]interface{})["key2"] = "value2") can panic if the fixture shape changes; replace them with explicit, checked extractions: first assert metadata, then annotations, then set the constants.AnnotationGeneration key, and separately assert data before setting "key2", failing the test (t.Fatalf or require) with a clear message if any assertion fails so failures are explicit and non-panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/executor/resource_executor.go`:
- Around line 167-169: The code currently logs and overwrites entries in
execCtx.Resources when a nested discovery name collides with any existing
resource (only explicitly skipping parent-name); change this to reject
collisions instead of silently replacing: when adding a nested discovery (check
where nestedName is used and where re.log.Warnf is called), first test if
execCtx.Resources[nestedName] already exists and if so return an error (or
record a failure) rather than overwrite, and apply the same change to the other
duplicate-handling block around the later re.log.Warnf (the second collision
site). Ensure the error includes the conflicting name (nestedName) and context
so callers can surface the collision instead of proceeding with a replaced
resource.
In `@test/integration/k8s_client/helper_envtest_prebuilt.go`:
- Around line 62-67: The current readiness retry loop returns immediately if
resp.Body.Close() fails, bypassing the retry/backoff; change the loop so that
Close errors are not returned immediately—capture or log the close error and
continue polling (keeping the existing statusCode check and retry/backoff
logic), and only return the close error if the overall deadline/retry limit is
reached; update the block around resp.Body.Close(), statusCode and the readiness
retry loop (the variables resp, statusCode and the readiness-checking function
or loop) to defer surfacing closeErr until after retries are exhausted.
In `@test/integration/testutil/container.go`:
- Around line 462-464: The container.Terminate calls in the shared-container
failure paths must use a 60-second timeout context and fall back to
forceCleanupContainerNoTest on failure; update the port mapping error path and
the retry cleanup path to create a terminateCtx with
context.WithTimeout(context.Background(), 60*time.Second), call
container.Terminate(terminateCtx) and if that returns an error then call
forceCleanupContainerNoTest(containerID) (and cancel the context) — mirror the
existing pattern used elsewhere in this file so both timeout and force-cleanup
fallback are applied consistently for container.Terminate.
- Around line 252-253: Replace plain exec.Command calls used for cleanup with
context-aware commands using context.WithTimeout(..., 30*time.Second) and
exec.CommandContext so cleanup won't hang; specifically change the rmCmd/cleanup
invocations in forceCleanupContainer, CleanupLeakedContainers, and
forceCleanupContainerNoTest to create a context with a 30s deadline, pass that
context into exec.CommandContext, and ensure you cancel the context and handle
the context deadline error paths consistently with existing timeout patterns in
this file.
---
Nitpick comments:
In `@internal/config_loader/struct_validator.go`:
- Around line 68-73: The RegisterValidation calls for custom validators
("resourcename" and "validoperator") are currently ignoring returned errors;
update the initialization in struct_validator.go to fail fast by checking the
returned error from structValidator.RegisterValidation for "resourcename"
(validateResourceName) and "validoperator" (validateOperator) and handling it
(e.g., log.Fatal / panic or return the error from the init function) so typos or
registration failures surface at startup rather than at runtime validation.
In `@test/integration/executor/executor_k8s_integration_test.go`:
- Around line 285-287: The test now needs an explicit assertion that the
outgoing status JSON uses the renamed key "clusterID" (not "clusterId"); update
the integration test in executor_k8s_integration_test.go to inspect the recorded
status payload (the same object you currently check for "conditions") and assert
that payload["clusterID"] equals the expected cluster ID value (the template
value used in the helper). Locate the place where you validate "conditions" and
add a similar assertion for "clusterID" so the contract change is covered by the
test.
In `@test/integration/maestro_client/client_integration_test.go`:
- Around line 223-225: The chained type assertions on configMapManifest (e.g.,
configMapManifest["metadata"].(map[string]interface{})["annotations"].(map[string]interface{})[constants.AnnotationGeneration]
= "2" and configMapManifest["data"].(map[string]interface{})["key2"] = "value2")
can panic if the fixture shape changes; replace them with explicit, checked
extractions: first assert metadata, then annotations, then set the
constants.AnnotationGeneration key, and separately assert data before setting
"key2", failing the test (t.Fatalf or require) with a clear message if any
assertion fails so failures are explicit and non-panicking.
In `@test/integration/testutil/mock_api_server.go`:
- Around line 82-85: Replace the fragile string comparison for EOF when reading
the request body: instead of checking readErr.Error() != "EOF", use
errors.Is(readErr, io.EOF) to detect EOF; update the condition around
r.Body.Read(buf) and t.Logf to only log non-EOF errors, and add the necessary
imports (errors and io) if they’re not already present so the check uses
errors.Is(readErr, io.EOF) with the existing variables readErr, r.Body.Read,
buf, and t.Logf.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce919fd6-8396-4117-82a9-dd24452edf6a
📒 Files selected for processing (60)
.golangci.ymlcmd/adapter/main.godocs/runbook.mdinternal/config_loader/loader.gointernal/config_loader/loader_test.gointernal/config_loader/struct_validator.gointernal/config_loader/types.gointernal/config_loader/validator.gointernal/config_loader/validator_test.gointernal/config_loader/viper_loader.gointernal/criteria/evaluator_test.gointernal/dryrun/discovery_overrides.gointernal/dryrun/dryrun_api_client.gointernal/dryrun/dryrun_responses.gointernal/dryrun/event_loader.gointernal/dryrun/event_loader_test.gointernal/dryrun/recording_transport_client.gointernal/dryrun/trace.gointernal/executor/executor.gointernal/executor/executor_test.gointernal/executor/param_extractor.gointernal/executor/post_action_executor.gointernal/executor/post_action_executor_test.gointernal/executor/precondition_executor.gointernal/executor/resource_executor.gointernal/executor/resource_executor_test.gointernal/executor/types.gointernal/executor/utils.gointernal/generation/generation.gointernal/hyperfleet_api/client.gointernal/hyperfleet_api/client_test.gointernal/k8s_client/apply.gointernal/k8s_client/client.gointernal/k8s_client/discovery.gointernal/k8s_client/interface.gointernal/k8s_client/mock.gointernal/k8s_client/test_helpers_test.gointernal/maestro_client/client.gointernal/maestro_client/interface.gointernal/maestro_client/operations_test.gointernal/manifest/generation.gointernal/manifest/manifestwork.gointernal/transport_client/interface.gopkg/errors/api_error.gopkg/errors/error.gopkg/errors/error_test.gopkg/logger/with_error_field_test.gopkg/otel/tracer.gotest/integration/config-loader/loader_template_test.gotest/integration/executor/executor_integration_test.gotest/integration/executor/executor_k8s_integration_test.gotest/integration/executor/main_test.gotest/integration/k8s_client/client_integration_test.gotest/integration/k8s_client/helper_envtest_prebuilt.gotest/integration/k8s_client/main_test.gotest/integration/maestro_client/client_integration_test.gotest/integration/maestro_client/client_tls_config_integration_test.gotest/integration/maestro_client/main_test.gotest/integration/testutil/container.gotest/integration/testutil/mock_api_server.go
💤 Files with no reviewable changes (1)
- .golangci.yml
rafabene
force-pushed
the
HYPERFLEET-769
branch
3 times, most recently
from
d884e07 to
11c8e9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/k8sclient/mock.go (1)
141-156:⚠️ Potential issue | 🟡 MinorMake the mock accept YAML manifests too.
The real
Client.ApplyResourcefalls back to YAML parsing, but this mock onlyjson.Unmarshals. Tests that pass YAML will fail against the mock while succeeding in production.🧪 Suggested fix
- obj := &unstructured.Unstructured{} - if err := json.Unmarshal(manifestBytes, &obj.Object); err != nil { + obj, err := parseToUnstructured(manifestBytes) + if err != nil { return nil, err }This also lets you drop the now-unused
encoding/jsonimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/k8sclient/mock.go` around lines 141 - 156, The mock ApplyResource currently only json.Unmarshal's manifestBytes and fails for YAML; update MockK8sClient.ApplyResource to attempt JSON unmarshal first into obj.Object and if that fails attempt YAML-to-JSON unmarshal (using sigs.k8s.io/yaml or equivalent) or yaml.Unmarshal into the same unstructured.Unstructured, so YAML manifests are accepted; keep existing error return behavior and still respect ApplyResourceError/ApplyResourceResult, and remove the now-unused encoding/json import if you switch to the YAML helper.
♻️ Duplicate comments (2)
test/integration/testutil/container.go (2)
461-466:⚠️ Potential issue | 🟠 MajorBound all shared-container terminate paths and keep force-cleanup fallback consistent.
Line 484 and Line 495 still call
container.Terminate(ctx)using unboundedcontext.Background(), and Line 462 only logs terminate failure withoutforceCleanupContainerNoTest. This can hangTestMainand leak containers when the runtime is unhealthy.Suggested fix
@@ - if container != nil { - terminateCtx, cancel := context.WithTimeout(context.Background(), 60*time.Second) - if termErr := container.Terminate(terminateCtx); termErr != nil { - println(fmt.Sprintf(" Failed to terminate container: %v", termErr)) - } - cancel() - } + if container != nil { + terminateCtx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + if termErr := container.Terminate(terminateCtx); termErr != nil { + println(fmt.Sprintf(" Failed to terminate container: %v", termErr)) + if cid := container.GetContainerID(); cid != "" { + forceCleanupContainerNoTest(cid) + } + } + cancel() + } @@ - if err != nil { - if termErr := container.Terminate(ctx); termErr != nil { - println(fmt.Sprintf(" Failed to terminate container after host error: %v", termErr)) - } + if err != nil { + terminateCtx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + if termErr := container.Terminate(terminateCtx); termErr != nil { + println(fmt.Sprintf(" Failed to terminate container after host error: %v", termErr)) + if cid := container.GetContainerID(); cid != "" { + forceCleanupContainerNoTest(cid) + } + } + cancel() return nil, fmt.Errorf("failed to get %s container host: %w", config.Name, err) } @@ - if err != nil { - if termErr := container.Terminate(ctx); termErr != nil { - println(fmt.Sprintf(" Failed to terminate container after port mapping error: %v", termErr)) - } + if err != nil { + terminateCtx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + if termErr := container.Terminate(terminateCtx); termErr != nil { + println(fmt.Sprintf(" Failed to terminate container after port mapping error: %v", termErr)) + if cid := container.GetContainerID(); cid != "" { + forceCleanupContainerNoTest(cid) + } + } + cancel() return nil, fmt.Errorf( "failed to get mapped port %s for %s container: %w", portSpec, config.Name, err, ) }Also applies to: 484-497
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/testutil/container.go` around lines 461 - 466, The terminate logic uses unbounded contexts and inconsistent fallback handling; replace calls to container.Terminate(context.Background()) with a bounded context (e.g., context.WithTimeout like terminateCtx) and ensure you call cancel() after use, and when container.Terminate returns an error use the same forceCleanupContainerNoTest fallback path currently used elsewhere (invoke forceCleanupContainerNoTest(container) or equivalent) instead of only logging; update the blocks around terminateCtx/terminateCtx, container.Terminate, and the error handling to mirror the pattern at lines 461–466 so all terminate paths are bounded and consistently trigger forceCleanupContainerNoTest on failure.
252-254:⚠️ Potential issue | 🟠 MajorUse timeout-bound
exec.CommandContextfor cleanup commands.These cleanup paths still use plain
exec.Commandwithout deadlines. Ifdocker/podmanhangs, test cleanup can block indefinitely (includingTestMaincleanup path).Suggested fix pattern (apply to all listed callsites)
@@ - rmCmd := exec.Command(runtime, "rm", "-f", containerID) + cmdCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + rmCmd := exec.CommandContext(cmdCtx, runtime, "rm", "-f", containerID) if output, err := rmCmd.CombinedOutput(); err == nil { + cancel() t.Logf("Force-removed container %s using %s", containerID, runtime) return } else { + cancel() t.Logf("Failed to force-remove with %s: %v (output: %s)", runtime, err, string(output)) } @@ - listCmd := exec.Command(runtime, "ps", "-a", "-q", "--filter", fmt.Sprintf("ancestor=%s", imagePattern)) + cmdCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + listCmd := exec.CommandContext(cmdCtx, runtime, "ps", "-a", "-q", "--filter", fmt.Sprintf("ancestor=%s", imagePattern)) output, err := listCmd.Output() + cancel() @@ - rmCmd := exec.Command(runtime, "rm", "-f", id) // `#nosec` G204 -- test infrastructure, commands constructed internally + cmdCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + rmCmd := exec.CommandContext(cmdCtx, runtime, "rm", "-f", id) // `#nosec` G204 -- test infrastructure, commands constructed internally if rmErr := rmCmd.Run(); rmErr != nil { + cancel() t.Logf("Warning: Failed to remove container %s: %v", id, rmErr) } else { + cancel() t.Logf("Cleaned up leaked container: %s", id) } @@ - rmCmd := exec.Command(runtime, "rm", "-f", containerID) + cmdCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + rmCmd := exec.CommandContext(cmdCtx, runtime, "rm", "-f", containerID) if _, err := rmCmd.CombinedOutput(); err == nil { + cancel() println(fmt.Sprintf(" Force-removed container %s using %s", containerID, runtime)) return } + cancel()Also applies to: 283-284, 300-301, 527-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/testutil/container.go` around lines 252 - 254, Replace uses of exec.Command for cleanup with exec.CommandContext using a context with a timeout to avoid hangs; for example, wrap the cleanup call that constructs rmCmd (and other similar cleanup calls creating runCmd/inspectCmd/etc.) with ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) and pass ctx into exec.CommandContext(runtime, "rm", "-f", containerID), defer cancel(), then use cmd.CombinedOutput() as before and handle the error/output. Apply the same pattern to the other cleanup callsites mentioned (the other exec.Command instances used for container cleanup).
🧹 Nitpick comments (2)
test/integration/maestroclient/client_tls_config_integration_test.go (1)
20-25: Move the Maestro config mapping into shared code instead of mirroringmain.go.This helper can drift from the runtime path in
cmd/adapter/main.go, so these tests may keep passing while the actual adapter wiring changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/maestroclient/client_tls_config_integration_test.go` around lines 20 - 25, The test helper buildMaestroClientConfigFromLoaded duplicates the mapping logic from createMaestroClient in cmd/adapter/main.go which risks drifting; refactor by extracting the mapping into a single shared, exported function (e.g., MaestroClientConfigFromLoader or NewMaestroClientConfig) that performs the mapping used by createMaestroClient and by tests, then have buildMaestroClientConfigFromLoaded call that shared function (or replace the test helper with a direct call) so both runtime wiring and tests use the exact same implementation.test/integration/executor/executor_k8s_integration_test.go (1)
145-146: Drop the unusedapiBaseURLparameter fromcreateK8sTestConfig.Line 146 explicitly discards it, but the call sites still pass
mockAPI.URL(), which makes the helper look more configurable than it is.♻️ Suggested cleanup
-func createK8sTestConfig(apiBaseURL, testNamespace string) *configloader.Config { - _ = apiBaseURL // Base URL is pulled from env params +func createK8sTestConfig(testNamespace string) *configloader.Config {Then update the callers in this file to use
createK8sTestConfig(testNamespace).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/executor/executor_k8s_integration_test.go` around lines 145 - 146, Remove the unused apiBaseURL parameter from createK8sTestConfig and update all local callers to match the new signature; specifically, change the function declaration from createK8sTestConfig(apiBaseURL, testNamespace string) to createK8sTestConfig(testNamespace string) and adjust every call in this test file that currently passes mockAPI.URL() so they only pass testNamespace (e.g., replace createK8sTestConfig(mockAPI.URL(), testNamespace) with createK8sTestConfig(testNamespace)); ensure any references to apiBaseURL inside createK8sTestConfig are deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/k8sclient/discovery.go`:
- Around line 28-38: The example call is using the old 3-argument signature for
Client.DiscoverResources; update the sample to call Client.DiscoverResources
with the current 4-parameter signature by providing a TransportContext as the
fourth argument (e.g., pass a nil or appropriate
transportclient.TransportContext value) so the example matches the function
DiscoverResources(ctx, gvk, discovery, transportCtx) and compiles against the
manifest.Discovery parameter and transportclient.TransportContext types.
In `@test/integration/k8sclient/README.md`:
- Around line 92-95: Update the fenced code block that shows the test output
(the block containing "PASS\nok
github.com/openshift-hyperfleet/hyperfleet-adapter/test/integration/k8sclient
26.148s") by adding the language identifier "text" after the opening backticks
so the block becomes ```text; this satisfies markdownlint MD040 while leaving
the content unchanged.
- Line 202: Update the markdown link label that currently reads "k8s_client
Package Documentation" to the new package name "k8sclient Package Documentation"
(i.e., change the visible link text while keeping the target path
"../../../internal/k8sclient/README.md" unchanged) so the README no longer
displays the stale "k8s_client" label.
---
Outside diff comments:
In `@internal/k8sclient/mock.go`:
- Around line 141-156: The mock ApplyResource currently only json.Unmarshal's
manifestBytes and fails for YAML; update MockK8sClient.ApplyResource to attempt
JSON unmarshal first into obj.Object and if that fails attempt YAML-to-JSON
unmarshal (using sigs.k8s.io/yaml or equivalent) or yaml.Unmarshal into the same
unstructured.Unstructured, so YAML manifests are accepted; keep existing error
return behavior and still respect ApplyResourceError/ApplyResourceResult, and
remove the now-unused encoding/json import if you switch to the YAML helper.
---
Duplicate comments:
In `@test/integration/testutil/container.go`:
- Around line 461-466: The terminate logic uses unbounded contexts and
inconsistent fallback handling; replace calls to
container.Terminate(context.Background()) with a bounded context (e.g.,
context.WithTimeout like terminateCtx) and ensure you call cancel() after use,
and when container.Terminate returns an error use the same
forceCleanupContainerNoTest fallback path currently used elsewhere (invoke
forceCleanupContainerNoTest(container) or equivalent) instead of only logging;
update the blocks around terminateCtx/terminateCtx, container.Terminate, and the
error handling to mirror the pattern at lines 461–466 so all terminate paths are
bounded and consistently trigger forceCleanupContainerNoTest on failure.
- Around line 252-254: Replace uses of exec.Command for cleanup with
exec.CommandContext using a context with a timeout to avoid hangs; for example,
wrap the cleanup call that constructs rmCmd (and other similar cleanup calls
creating runCmd/inspectCmd/etc.) with ctx, cancel :=
context.WithTimeout(context.Background(), time.Second*5) and pass ctx into
exec.CommandContext(runtime, "rm", "-f", containerID), defer cancel(), then use
cmd.CombinedOutput() as before and handle the error/output. Apply the same
pattern to the other cleanup callsites mentioned (the other exec.Command
instances used for container cleanup).
---
Nitpick comments:
In `@test/integration/executor/executor_k8s_integration_test.go`:
- Around line 145-146: Remove the unused apiBaseURL parameter from
createK8sTestConfig and update all local callers to match the new signature;
specifically, change the function declaration from
createK8sTestConfig(apiBaseURL, testNamespace string) to
createK8sTestConfig(testNamespace string) and adjust every call in this test
file that currently passes mockAPI.URL() so they only pass testNamespace (e.g.,
replace createK8sTestConfig(mockAPI.URL(), testNamespace) with
createK8sTestConfig(testNamespace)); ensure any references to apiBaseURL inside
createK8sTestConfig are deleted.
In `@test/integration/maestroclient/client_tls_config_integration_test.go`:
- Around line 20-25: The test helper buildMaestroClientConfigFromLoaded
duplicates the mapping logic from createMaestroClient in cmd/adapter/main.go
which risks drifting; refactor by extracting the mapping into a single shared,
exported function (e.g., MaestroClientConfigFromLoader or
NewMaestroClientConfig) that performs the mapping used by createMaestroClient
and by tests, then have buildMaestroClientConfigFromLoaded call that shared
function (or replace the test helper with a direct call) so both runtime wiring
and tests use the exact same implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccd06a99-1556-4659-8051-5f08c54722f4
📒 Files selected for processing (87)
.golangci.ymlREADME.mdcmd/adapter/main.godocs/runbook.mdinternal/configloader/README.mdinternal/configloader/accessors.gointernal/configloader/constants.gointernal/configloader/loader.gointernal/configloader/loader_test.gointernal/configloader/struct_validator.gointernal/configloader/types.gointernal/configloader/validator.gointernal/configloader/validator_test.gointernal/configloader/viper_loader.gointernal/criteria/README.mdinternal/criteria/evaluator_test.gointernal/dryrun/discovery_overrides.gointernal/dryrun/dryrun_api_client.gointernal/dryrun/dryrun_api_client_test.gointernal/dryrun/dryrun_responses.gointernal/dryrun/event_loader.gointernal/dryrun/event_loader_test.gointernal/dryrun/recording_transport_client.gointernal/dryrun/recording_transport_client_test.gointernal/dryrun/trace.gointernal/executor/executor.gointernal/executor/executor_test.gointernal/executor/param_extractor.gointernal/executor/post_action_executor.gointernal/executor/post_action_executor_test.gointernal/executor/precondition_executor.gointernal/executor/resource_executor.gointernal/executor/resource_executor_test.gointernal/executor/types.gointernal/executor/utils.gointernal/executor/utils_test.gointernal/generation/generation.gointernal/hyperfleetapi/README.mdinternal/hyperfleetapi/client.gointernal/hyperfleetapi/client_test.gointernal/hyperfleetapi/mock.gointernal/hyperfleetapi/types.gointernal/k8sclient/README.mdinternal/k8sclient/apply.gointernal/k8sclient/client.gointernal/k8sclient/client_test.gointernal/k8sclient/discovery.gointernal/k8sclient/interface.gointernal/k8sclient/mock.gointernal/k8sclient/test_helpers_test.gointernal/k8sclient/types.gointernal/k8sclient/types_test.gointernal/maestroclient/client.gointernal/maestroclient/client_test.gointernal/maestroclient/interface.gointernal/maestroclient/ocm_logger_adapter.gointernal/maestroclient/operations.gointernal/maestroclient/operations_test.gointernal/manifest/generation.gointernal/manifest/manifestwork.gointernal/transportclient/interface.gointernal/transportclient/types.gopkg/errors/api_error.gopkg/errors/error.gopkg/errors/error_test.gopkg/logger/with_error_field_test.gopkg/otel/tracer.gotest/integration/README.mdtest/integration/config-loader/config_criteria_integration_test.gotest/integration/config-loader/loader_template_test.gotest/integration/executor/executor_integration_test.gotest/integration/executor/executor_k8s_integration_test.gotest/integration/executor/main_test.gotest/integration/executor/setup_test.gotest/integration/k8sclient/README.mdtest/integration/k8sclient/client_integration_test.gotest/integration/k8sclient/helper_envtest_prebuilt.gotest/integration/k8sclient/helper_selector.gotest/integration/k8sclient/main_test.gotest/integration/maestroclient/client_integration_test.gotest/integration/maestroclient/client_tls_config_integration_test.gotest/integration/maestroclient/client_tls_integration_test.gotest/integration/maestroclient/main_test.gotest/integration/maestroclient/setup_test.gotest/integration/maestroclient/tls_helper_test.gotest/integration/testutil/container.gotest/integration/testutil/mock_api_server.go
💤 Files with no reviewable changes (1)
- .golangci.yml
✅ Files skipped from review due to trivial changes (47)
- internal/hyperfleetapi/types.go
- internal/maestroclient/client_test.go
- internal/configloader/README.md
- internal/maestroclient/ocm_logger_adapter.go
- internal/dryrun/dryrun_responses.go
- test/integration/maestroclient/tls_helper_test.go
- internal/configloader/struct_validator.go
- test/integration/maestroclient/setup_test.go
- internal/criteria/README.md
- internal/hyperfleetapi/mock.go
- README.md
- internal/dryrun/discovery_overrides.go
- internal/configloader/accessors.go
- internal/maestroclient/operations_test.go
- pkg/errors/error_test.go
- internal/configloader/constants.go
- docs/runbook.md
- internal/manifest/manifestwork.go
- internal/transportclient/types.go
- test/integration/README.md
- internal/dryrun/event_loader_test.go
- test/integration/k8sclient/client_integration_test.go
- internal/maestroclient/operations.go
- internal/transportclient/interface.go
- internal/hyperfleetapi/client_test.go
- internal/k8sclient/types_test.go
- internal/k8sclient/README.md
- internal/configloader/loader_test.go
- internal/dryrun/dryrun_api_client_test.go
- internal/dryrun/trace.go
- internal/criteria/evaluator_test.go
- internal/hyperfleetapi/README.md
- internal/dryrun/event_loader.go
- internal/configloader/viper_loader.go
- test/integration/testutil/mock_api_server.go
- pkg/logger/with_error_field_test.go
- internal/k8sclient/test_helpers_test.go
- internal/manifest/generation.go
- internal/maestroclient/interface.go
- internal/k8sclient/types.go
- pkg/errors/api_error.go
- internal/configloader/types.go
- internal/generation/generation.go
- internal/executor/utils_test.go
- pkg/otel/tracer.go
- internal/executor/param_extractor.go
- internal/k8sclient/client_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- test/integration/executor/main_test.go
- internal/executor/resource_executor_test.go
- pkg/errors/error.go
- internal/executor/executor.go
- test/integration/executor/executor_integration_test.go
- internal/executor/types.go
- internal/executor/resource_executor.go
- internal/executor/post_action_executor.go
- internal/dryrun/dryrun_api_client.go
- internal/executor/precondition_executor.go
- cmd/adapter/main.go
Align .golangci.yml with the architecture standard by removing all repo-specific exclusions and fixing the underlying code: - Remove misspell, gocritic, gosec, and revive exclusions - Fix all lint violations: filepath.Clean, crypto/rand, line length, if/else->switch, exitAfterDefer, var-naming, errcheck, goconst - Rename underscore packages to comply with Go naming conventions: config_loader->configloader, hyperfleet_api->hyperfleetapi, k8s_client->k8sclient, maestro_client->maestroclient, transport_client->transportclient - Rename integration test packages to remove underscores - Fix k8s_client integration tests failing in -short mode - Reject nested discovery key collisions instead of silently overwriting - Use context-aware exec.CommandContext with timeouts for cleanup commands - Add forceCleanupContainerNoTest fallback on container.Terminate failure - Check RegisterValidation errors at startup (fail fast) - Add clusterID assertion in executor K8s integration test - Replace chained type assertions with checked extractions in maestro test - Use errors.Is(readErr, io.EOF) instead of string comparison - Add YAML fallback in MockK8sClient.ApplyResource - Fix DiscoverResources example to match 4-param signature - Remove unused apiBaseURL param from createK8sTestConfig Only remaining exclusions: pkg/errors and pkg/utils naming (renaming would break the public API surface).
rafabene
force-pushed
the
HYPERFLEET-769
branch
from
11c8e9c to
b236c76
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/integration/maestroclient/client_tls_config_integration_test.go (1)
20-25: This helper can drift from the runtime mapping.The suite rebuilds the loader →
maestroclient.Configconversion locally, so a production wiring regression can still leave these tests green. Prefer extracting that mapping into a shared helper and using it from both the runtime path and this test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/maestroclient/client_tls_config_integration_test.go` around lines 20 - 25, The test duplicates the loader→maestroclient.Config mapping in buildMaestroClientConfigFromLoaded which can drift from the runtime createMaestroClient wiring; refactor the mapping into a shared helper (e.g., NewMaestroClientConfigFromLoader) that accepts configloader.MaestroClientConfig and returns *maestroclient.Config/error, use that helper from cmd/adapter/main.go (replace createMaestroClient’s inline mapping) and call the same helper from the test instead of buildMaestroClientConfigFromLoaded, removing the duplicated logic to ensure a single source of truth.internal/k8sclient/mock.go (1)
155-167: Error variable shadowing may cause confusion.On line 163,
erris redeclared with:=which shadows the outererrfrom line 157. While the current logic works (the outererris used in the error message on line 161, and the innererris used on line 164), this pattern can be confusing and error-prone.Consider using a different variable name for the inner error to improve clarity:
♻️ Suggested improvement for clarity
obj := &unstructured.Unstructured{} - if err := json.Unmarshal(manifestBytes, &obj.Object); err != nil { + if jsonErr := json.Unmarshal(manifestBytes, &obj.Object); jsonErr != nil { jsonBytes, yamlErr := yaml.YAMLToJSON(manifestBytes) if yamlErr != nil { return nil, fmt.Errorf( - "failed to parse manifest as JSON or YAML: %w", err) + "failed to parse manifest as JSON or YAML: json=%v, yaml=%v", jsonErr, yamlErr) } - if err := json.Unmarshal(jsonBytes, &obj.Object); err != nil { + if unmarshalErr := json.Unmarshal(jsonBytes, &obj.Object); unmarshalErr != nil { return nil, fmt.Errorf( - "failed to parse manifest after YAML conversion: %w", err) + "failed to parse manifest after YAML conversion: %w", unmarshalErr) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/k8sclient/mock.go` around lines 155 - 167, The JSON/YAML parsing block shadows the outer err by redeclaring err during the second json.Unmarshal, which is confusing; change the inner error variable to a distinct name (e.g., jsonErr) when unmarshalling the JSON produced by yaml.YAMLToJSON and use that jsonErr in the fmt.Errorf for the "failed to parse manifest after YAML conversion" message, keeping the original outer err for the initial JSON parse failure; locate the parsing logic around manifestBytes, obj (*unstructured.Unstructured), json.Unmarshal and yaml.YAMLToJSON to make this small renaming fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/executor/resource_executor.go`:
- Around line 175-181: The collision branch currently returns
NewExecutorError(...) without marking result as failed or notifying the adapter,
leaving result.Status as StatusSuccess and skipping
execCtx.Adapter.ExecutionError; update the branch handling where
execCtx.Resources[nestedName] is detected to set result.Status to a failure
status (e.g., StatusFailed), populate result.Error with the produced error, call
execCtx.Adapter.ExecutionError(ctx, resource.Name, err) (or the appropriate
adapter method) and then return result and the error from
NewExecutorError(PhaseResources, resource.Name, ...); ensure you reference the
same symbols (execCtx.Resources, nestedName, NewExecutorError, PhaseResources,
resource.Name, result, StatusSuccess, result.Error,
execCtx.Adapter.ExecutionError) so the resource collision is recorded
consistently with other failure paths.
---
Nitpick comments:
In `@internal/k8sclient/mock.go`:
- Around line 155-167: The JSON/YAML parsing block shadows the outer err by
redeclaring err during the second json.Unmarshal, which is confusing; change the
inner error variable to a distinct name (e.g., jsonErr) when unmarshalling the
JSON produced by yaml.YAMLToJSON and use that jsonErr in the fmt.Errorf for the
"failed to parse manifest after YAML conversion" message, keeping the original
outer err for the initial JSON parse failure; locate the parsing logic around
manifestBytes, obj (*unstructured.Unstructured), json.Unmarshal and
yaml.YAMLToJSON to make this small renaming fix.
In `@test/integration/maestroclient/client_tls_config_integration_test.go`:
- Around line 20-25: The test duplicates the loader→maestroclient.Config mapping
in buildMaestroClientConfigFromLoaded which can drift from the runtime
createMaestroClient wiring; refactor the mapping into a shared helper (e.g.,
NewMaestroClientConfigFromLoader) that accepts configloader.MaestroClientConfig
and returns *maestroclient.Config/error, use that helper from
cmd/adapter/main.go (replace createMaestroClient’s inline mapping) and call the
same helper from the test instead of buildMaestroClientConfigFromLoaded,
removing the duplicated logic to ensure a single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afa1196b-efdb-4dc8-be57-b9fa68022ec5
📒 Files selected for processing (87)
.golangci.ymlREADME.mdcmd/adapter/main.godocs/runbook.mdinternal/configloader/README.mdinternal/configloader/accessors.gointernal/configloader/constants.gointernal/configloader/loader.gointernal/configloader/loader_test.gointernal/configloader/struct_validator.gointernal/configloader/types.gointernal/configloader/validator.gointernal/configloader/validator_test.gointernal/configloader/viper_loader.gointernal/criteria/README.mdinternal/criteria/evaluator_test.gointernal/dryrun/discovery_overrides.gointernal/dryrun/dryrun_api_client.gointernal/dryrun/dryrun_api_client_test.gointernal/dryrun/dryrun_responses.gointernal/dryrun/event_loader.gointernal/dryrun/event_loader_test.gointernal/dryrun/recording_transport_client.gointernal/dryrun/recording_transport_client_test.gointernal/dryrun/trace.gointernal/executor/executor.gointernal/executor/executor_test.gointernal/executor/param_extractor.gointernal/executor/post_action_executor.gointernal/executor/post_action_executor_test.gointernal/executor/precondition_executor.gointernal/executor/resource_executor.gointernal/executor/resource_executor_test.gointernal/executor/types.gointernal/executor/utils.gointernal/executor/utils_test.gointernal/generation/generation.gointernal/hyperfleetapi/README.mdinternal/hyperfleetapi/client.gointernal/hyperfleetapi/client_test.gointernal/hyperfleetapi/mock.gointernal/hyperfleetapi/types.gointernal/k8sclient/README.mdinternal/k8sclient/apply.gointernal/k8sclient/client.gointernal/k8sclient/client_test.gointernal/k8sclient/discovery.gointernal/k8sclient/interface.gointernal/k8sclient/mock.gointernal/k8sclient/test_helpers_test.gointernal/k8sclient/types.gointernal/k8sclient/types_test.gointernal/maestroclient/client.gointernal/maestroclient/client_test.gointernal/maestroclient/interface.gointernal/maestroclient/ocm_logger_adapter.gointernal/maestroclient/operations.gointernal/maestroclient/operations_test.gointernal/manifest/generation.gointernal/manifest/manifestwork.gointernal/transportclient/interface.gointernal/transportclient/types.gopkg/errors/api_error.gopkg/errors/error.gopkg/errors/error_test.gopkg/logger/with_error_field_test.gopkg/otel/tracer.gotest/integration/README.mdtest/integration/config-loader/config_criteria_integration_test.gotest/integration/config-loader/loader_template_test.gotest/integration/executor/executor_integration_test.gotest/integration/executor/executor_k8s_integration_test.gotest/integration/executor/main_test.gotest/integration/executor/setup_test.gotest/integration/k8sclient/README.mdtest/integration/k8sclient/client_integration_test.gotest/integration/k8sclient/helper_envtest_prebuilt.gotest/integration/k8sclient/helper_selector.gotest/integration/k8sclient/main_test.gotest/integration/maestroclient/client_integration_test.gotest/integration/maestroclient/client_tls_config_integration_test.gotest/integration/maestroclient/client_tls_integration_test.gotest/integration/maestroclient/main_test.gotest/integration/maestroclient/setup_test.gotest/integration/maestroclient/tls_helper_test.gotest/integration/testutil/container.gotest/integration/testutil/mock_api_server.go
✅ Files skipped from review due to trivial changes (38)
- internal/configloader/README.md
- README.md
- internal/criteria/evaluator_test.go
- internal/dryrun/discovery_overrides.go
- internal/hyperfleetapi/mock.go
- internal/hyperfleetapi/README.md
- internal/k8sclient/client_test.go
- internal/maestroclient/client_test.go
- internal/maestroclient/ocm_logger_adapter.go
- docs/runbook.md
- internal/criteria/README.md
- internal/configloader/accessors.go
- internal/dryrun/dryrun_api_client_test.go
- internal/dryrun/trace.go
- internal/dryrun/dryrun_responses.go
- internal/executor/param_extractor.go
- internal/hyperfleetapi/types.go
- internal/k8sclient/README.md
- internal/k8sclient/types_test.go
- internal/manifest/manifestwork.go
- pkg/errors/error_test.go
- test/integration/README.md
- test/integration/k8sclient/README.md
- internal/maestroclient/operations.go
- pkg/otel/tracer.go
- internal/dryrun/recording_transport_client_test.go
- internal/configloader/validator_test.go
- internal/dryrun/event_loader_test.go
- internal/executor/resource_executor_test.go
- internal/manifest/generation.go
- test/integration/k8sclient/client_integration_test.go
- pkg/logger/with_error_field_test.go
- internal/k8sclient/test_helpers_test.go
- internal/generation/generation.go
- pkg/errors/api_error.go
- internal/transportclient/interface.go
- internal/executor/utils_test.go
- internal/maestroclient/interface.go
🚧 Files skipped from review as they are similar to previous changes (32)
- internal/dryrun/event_loader.go
- internal/k8sclient/types.go
- internal/configloader/constants.go
- internal/configloader/loader_test.go
- internal/configloader/validator.go
- internal/configloader/struct_validator.go
- internal/maestroclient/operations_test.go
- test/integration/testutil/mock_api_server.go
- test/integration/maestroclient/setup_test.go
- internal/transportclient/types.go
- pkg/errors/error.go
- internal/configloader/loader.go
- internal/configloader/types.go
- internal/configloader/viper_loader.go
- internal/hyperfleetapi/client_test.go
- internal/k8sclient/client.go
- test/integration/k8sclient/main_test.go
- test/integration/config-loader/config_criteria_integration_test.go
- test/integration/maestroclient/tls_helper_test.go
- test/integration/executor/setup_test.go
- test/integration/maestroclient/main_test.go
- internal/k8sclient/discovery.go
- cmd/adapter/main.go
- test/integration/testutil/container.go
- internal/executor/executor_test.go
- test/integration/maestroclient/client_integration_test.go
- internal/dryrun/recording_transport_client.go
- test/integration/executor/main_test.go
- internal/k8sclient/apply.go
- internal/executor/post_action_executor.go
- test/integration/maestroclient/client_tls_integration_test.go
- internal/dryrun/dryrun_api_client.go
…ision Align the collision error path with all other failure paths in executeResource by setting result.Status, result.Error, and execCtx.Adapter.ExecutionError before returning.
… error When a precondition API call fails, SetError() correctly sets adapter.executionStatus to "failed". However, SetSkipped() was called immediately after, overwriting executionStatus back to "success". This caused CEL expressions checking adapter.executionStatus (e.g., Health condition) to incorrectly evaluate as "True" instead of "False". Replace the SetSkipped() call in the precondition error path with direct field assignments that set resourcesSkipped and skipReason without overwriting the failed execution status.
Contributor
|
/rebase |
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.
Summary
.golangci.ymlwith the architecture standard by removing all repo-specific exclusions and fixing the underlying code (~185+ lint violations across 87 files)cancelled), gocritic (ifElseChain,singleCaseSwitch,elseif,exitAfterDefer), gosec (G304/G703,G404,G204), and all repo-specificissues.exclude-rulesfilepath.Clean()for file reads,crypto/randmigration, long line breaks,if/else→switchrefactors,exitAfterDefercorrections,clusterId→clusterIDrenames, errcheck/goconst fixesconfig_loader→configloaderhyperfleet_api→hyperfleetapik8s_client→k8sclientmaestro_client→maestroclienttransport_client→transportclientk8sclient_integration→k8sclientintegration)-shortmodeexec.CommandContextwith 30s timeouts for cleanup commandsforceCleanupContainerNoTestfallback oncontainer.TerminatefailureRegisterValidationerrors at startup (fail fast)clusterIDassertion in executor K8s integration testerrors.Is(readErr, io.EOF)instead of string comparisonOnly remaining exclusions
pkg/errorsandpkg/utilsnaming — renaming would break the public API surface. YAML config keys (e.g.,hyperfleet_api) are unchanged as they are configuration structure, not Go package names.Test plan
golangci-lint run ./...→ 0 issuesgo test ./internal/... ./pkg/... ./cmd/... -count=1 -short→ all passgo test ./test/integration/... -count=1 -short→ all passSummary by CodeRabbit
Code Quality
Documentation
Bug Fixes & Improvements