HYPERFLEET-652 - feat: dry-run mode#49
Merged
ciaranRoche merged 1 commit intoopenshift-hyperfleet:mainfrom Feb 18, 2026
Merged
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 |
WalkthroughAdds a local dry-run mode and documentation, refactors adapter startup to extract shared config/client/runner builders, and implements an internal dryrun package with: a mock HyperFleet API client, an in-memory transport client with operation recording and discovery overrides, loaders for CloudEvents and mock API responses, and an execution trace subsystem (text/JSON). Adds tests and testdata for dry-run inputs/outputs. Adjusts executor to capture resource identity fields for trace reporting and reuses the builder/executor pattern for both serve and dry-run paths. Sequence Diagram(s)sequenceDiagram
participant User
participant Main as cmd/adapter/main.go
participant Config as Config Loader
participant Event as Dryrun Event Loader
participant API as Dryrun API Client
participant Transport as Dryrun Transport Client
participant Executor as Executor
participant Trace as Trace Formatter
participant Output as File/Stdout
User->>Main: invoke with --dry-run-event / --dry-run-api-responses / --dry-run-discovery
Main->>Config: loadConfig(...)
Config-->>Main: config
Main->>Event: LoadCloudEvent(path)
Event-->>Main: CloudEvent
Main->>API: NewDryrunAPIClient(responses)
API-->>Main: dry-run API client
Main->>Transport: NewDryrunTransportClient(with overrides)
Transport-->>Main: dry-run transport client
Main->>Executor: buildExecutor(config, API, Transport)
Executor-->>Main: executor ready
Main->>Executor: Execute(event)
Executor->>API: Do(request)
API-->>Executor: mock response
Executor->>Transport: ApplyResource(manifest)
Transport-->>Executor: ApplyResult + record
Executor->>Transport: DiscoverResources(gvk)
Transport-->>Executor: discovered resources
Executor-->>Main: ExecutionResult
Main->>Trace: FormatText/FormatJSON(result)
Trace-->>Main: formatted trace
Main->>Output: write trace
Output-->>User: trace file / stdout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
Comment |
rh-amarin
force-pushed
the
new-dryrun
branch
from
ac4fd92 to
b52bb5a
Compare
![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
🧹 Nitpick comments (1)
internal/dryrun/event_loader_test.go (1)
43-53: Consider adding a test for CloudEvent validation failure.The tests cover file-not-found and invalid JSON, but there's no test for the
evt.Validate()error path (Line 25-27 in event_loader.go). A CloudEvent with valid JSON but missing required fields (e.g., missingspecversionortype) would exercise this branch.💡 Example test case
func TestLoadCloudEvent_InvalidCloudEvent(t *testing.T) { t.Run("returns error for valid JSON but invalid CloudEvent", func(t *testing.T) { dir := t.TempDir() // Valid JSON but missing required CloudEvent fields path := writeEventFile(t, dir, "invalid-event.json", `{"id":"test-123"}`) _, err := LoadCloudEvent(path) require.Error(t, err) assert.Contains(t, err.Error(), "invalid CloudEvent") }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/dryrun/event_loader_test.go` around lines 43 - 53, Add a unit test that exercises the CloudEvent validation error path in LoadCloudEvent by writing a file containing syntactically valid JSON that lacks required CloudEvent fields (e.g., missing "specversion" or "type"), calling LoadCloudEvent on that path, and asserting an error is returned and its message contains an indication of an invalid CloudEvent; reference the test helper writeEventFile and the function under test LoadCloudEvent/evt.Validate to locate where to add the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/adapter/main.go`:
- Around line 646-655: The switch over dryRunOutput silently falls back to text
for unknown values; validate the flag and fail fast instead. Update the handling
around the dryRunOutput variable used with trace.FormatJSON() and
trace.FormatText() to explicitly accept only supported values (e.g., "json" and
"text") and return a descriptive error for any other value rather than using the
default branch; alternatively add validation where the flag is parsed and return
fmt.Errorf("invalid --dry-run-output: %q, allowed: json,text") so callers see a
clear failure.
In `@internal/dryrun/dryrun_api_client.go`:
- Around line 3-35: The file fails gofmt; reformat
internal/dryrun/dryrun_api_client.go using gofmt (or goimports) so imports,
struct declarations (e.g., DryrunAPIClient, compiledEndpoint) and all
spacing/indentation adhere to gofmt rules; run `gofmt -w` (or `gofmt -w &&
goimports -w`) on the file and commit the resulting formatting changes to
resolve the linter error.
In `@internal/dryrun/recording_transport_client_test.go`:
- Around line 26-29: The method declarations on testDiscovery (GetNamespace,
GetName, GetLabelSelector, IsSingleResource) have extra spaces before their
opening braces causing gofmt failures; fix by removing the extra spaces so each
signature directly precedes the `{` (e.g., change `func (d *testDiscovery)
GetNamespace() string {` to `func (d *testDiscovery) GetNamespace() string
{`) or simply run `gofmt -w` on the file to auto-apply the formatting for all
these methods.
In `@internal/dryrun/recording_transport_client.go`:
- Around line 145-180: DiscoverResources in DryrunTransportClient currently
filters resources only by GVK, namespace and name, ignoring label selectors from
manifest.Discovery; update DiscoverResources to apply
discovery.GetLabelSelector() (when non-empty) to each object's labels
(obj.GetLabels()) and skip objects that don't match the selector, ensuring
behavior matches real discovery; use the existing loop in DiscoverResources and
add a label-match check before appending to list.Items so label selectors are
respected.
- Around line 60-75: The ApplyResource method in DryrunTransportClient currently
only calls json.Unmarshal on manifestBytes; change it to mirror
K8sClient/MaestroClient by attempting JSON first and, if that fails, falling
back to YAML parsing (e.g., use the same yaml.Unmarshal/sigs.k8s.io/yaml logic
those clients use) to populate obj.Object; update the error handling to record
the parse error only after both JSON and YAML attempts fail and ensure
TransportRecord.Manifest still stores the original manifestBytes.
In `@internal/dryrun/trace.go`:
- Around line 88-262: In ExecutionTrace.FormatText update the verbose manifest
lookup that iterates t.Transport.Records (the block that currently matches on
tr.Operation=="apply" && tr.Name==rr.ResourceName && tr.Namespace==rr.Namespace)
to also match the resource Kind (e.g., compare tr.Kind to rr.Kind, using
strings.EqualFold for case-insensitive comparison) so manifests are only shown
for the exact Kind/Namespace/Name triple and avoid collisions across kinds; keep
the rest of the logic (verbose rendering via tr.Manifest and break) unchanged.
In `@README.md`:
- Around line 207-212: Change the misspelled word "overriden" to "overridden" in
the README text describing the Adapter configuration (the line that mentions
"Configure topic/subscription in the `adapter-config.yaml` but can be overriden
with env variables or cli params"); update that single word so the sentence
reads "...but can be overridden with env variables or cli params".
In `@test/testdata/dryrun/dryrun-discovery.json`:
- Around line 30-37: The ConfigMap metadata contains a non-standard field
"ciaran": "roche" under the metadata object for the resource named
"abc123-symbol222"; remove this invalid metadata field or move it into a
standard metadata subsection such as metadata.annotations (e.g.,
metadata.annotations["ciaran"]="roche") if the intent is to preserve it for
testing; update any test documentation or comments to indicate it's an
intentional edge-case if kept.
In `@test/testdata/dryrun/task-config.yaml`:
- Around line 244-248: The apiCall for the "update-status" task is using the
singular path "/api/hyperfleet/v1/clusters/{{ .clusterId }}/status" which won't
match the mock API (the mock checks for "/statuses"); change the apiCall.url
value for the "update-status" task to use the plural
"/api/hyperfleet/v1/clusters/{{ .clusterId }}/statuses" and then update any
corresponding mock response pattern in the dryrun-api-responses.json file so the
mock handler (which looks for paths ending with "/statuses") will match.
---
Nitpick comments:
In `@internal/dryrun/event_loader_test.go`:
- Around line 43-53: Add a unit test that exercises the CloudEvent validation
error path in LoadCloudEvent by writing a file containing syntactically valid
JSON that lacks required CloudEvent fields (e.g., missing "specversion" or
"type"), calling LoadCloudEvent on that path, and asserting an error is returned
and its message contains an indication of an invalid CloudEvent; reference the
test helper writeEventFile and the function under test
LoadCloudEvent/evt.Validate to locate where to add the new test.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/dryrun/dryrun_responses.go (1)
45-52: ValidateurlPatternregex upfront to fail fast on invalid patterns.
Catching regex compile errors here prevents later runtime surprises in matching logic.♻️ Proposed change
import ( "encoding/json" "fmt" "os" + "regexp" ) @@ for i, ep := range mrf.Responses { if len(ep.Responses) == 0 { return nil, fmt.Errorf("dryrun responses file %q: endpoint %d has no responses defined", path, i) } if ep.Match.URLPattern == "" { return nil, fmt.Errorf("dryrun responses file %q: endpoint %d has empty urlPattern", path, i) } + if _, err := regexp.Compile(ep.Match.URLPattern); err != nil { + return nil, fmt.Errorf("dryrun responses file %q: endpoint %d has invalid urlPattern: %w", path, i, err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/dryrun/dryrun_responses.go` around lines 45 - 52, Loop over mrf.Responses in the existing validation loop and validate each ep.Match.URLPattern by attempting to compile it (e.g., using regexp.Compile); if compilation fails return a descriptive error including the pattern and the regex error so the function fails fast on invalid patterns. Keep this check adjacent to the existing emptiness check for ep.Match.URLPattern in the same loop that iterates mrf.Responses so malformed regexes are caught early.test/testdata/dryrun/task-config.yaml (1)
52-55: Resolve the TODO for the timestamp expression.
If you want, I can help validate a working expression (e.g., an equivalent ofnow) for this config format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/testdata/dryrun/task-config.yaml` around lines 52 - 55, The timestamp field currently hardcodes a format string instead of producing a current timestamp; locate the task entry with name "timestamp" and replace the expression value so it invokes the runtime's "now" function (not a quoted format literal) and formats it as an RFC3339 string; e.g., remove the escaped quotes and use the config's expression syntax to call now() (or the template engine's {{ now }} equivalent) with an RFC3339 formatter so the expression returns a concrete timestamp rather than the literal format string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/adapter/main.go`:
- Around line 171-174: The isDryRun function currently returns true if either
dryRunEvent or dryRunAPIResponses is non-empty, which can trigger dry-run mode
without an event file and later fail when the code expects an event; update
isDryRun (and any callers if needed) so it only considers dryRunEvent (i.e.,
return true only when dryRunEvent is non-empty) and leave dryRunAPIResponses
used only to toggle API-response recording behavior where those code paths check
that an event exists (see references to isDryRun, dryRunEvent, and
dryRunAPIResponses).
In `@internal/dryrun/dryrun_api_client.go`:
- Around line 79-86: nextResponse can panic when compiledEndpoint.ep.resps is
empty; add a guard at the start of the nextResponse method to handle
len(ep.resps) == 0 (instead of computing idx = -1). For example, return a
sensible zero-value DryrunResponse (or an explicit error/marker) and avoid
advancing ep.callIdx when there are no responses; alternatively validate and
reject empty Responses in NewDryrunAPIClient during construction. Update
nextResponse (and/or NewDryrunAPIClient) to check ep.resps length before
indexing and use the symbols nextResponse, compiledEndpoint, ep.resps,
ep.callIdx, DryrunResponse, and NewDryrunAPIClient to locate and fix the code.
In `@test/testdata/dryrun/dryrun-api-responses.json`:
- Around line 39-53: The mock API entry under the "match" object uses the wrong
singular path; update the "urlPattern" value in the JSON object (the entry with
"match":{"method":"PATCH","urlPattern":...}) to use
"/api/hyperfleet/v1/clusters/.*/statuses" (plural) instead of
"/api/hyperfleet/v1/clusters/.*/status" so it matches the HyperFleet API
contract and other tests.
---
Duplicate comments:
In `@cmd/adapter/main.go`:
- Around line 646-655: dryRunOutput currently falls through to text for unknown
values; update validation so only "json" and "text" are allowed and return an
error for anything else. Modify the switch around dryRunOutput (the branch using
trace.FormatJSON() and trace.FormatText()) to explicitly handle "json" and
"text" and have the default case return a descriptive error (e.g. invalid
--dry-run-output value) or pre-validate dryRunOutput before the switch so
callers fail fast on invalid values.
In `@internal/dryrun/recording_transport_client.go`:
- Around line 60-75: In ApplyResource of DryrunTransportClient, replace the
direct json.Unmarshal into obj.Object with YAML-to-JSON conversion so the method
accepts YAML as well as JSON: call sigs.k8s.io/yaml.YAMLToJSON (or yaml.ToJSON)
on manifestBytes and then json.Unmarshal the resulting JSON into obj.Object;
ensure errors from YAML conversion or JSON unmarshal are recorded in
TransportRecord.Error (keep the same Operation/Manifest fields) and returned,
and keep locking and appending to c.Records unchanged.
In `@README.md`:
- Around line 207-214: Replace the misspelled word "overriden" with the correct
spelling "overridden" in the README.md paragraph that describes adapter
configuration (the sentence mentioning "Configure topic/subscription in the
`adapter-config.yaml` but can be overriden with env variables or cli params");
update that sentence so it reads "overridden" and ensure surrounding
punctuation/capitalization remains unchanged.
In `@test/testdata/dryrun/dryrun-discovery.json`:
- Around line 30-34: The JSON includes a non-standard Kubernetes metadata field
"metadata.ciaran"; either remove this custom key or move it under
"metadata.annotations" as a string entry (e.g., "metadata.annotations.ciaran")
so it conforms to Kubernetes metadata schema; update any references to the
previous "metadata.ciaran" key in tests or fixtures to read from
"metadata.annotations" instead.
In `@test/testdata/dryrun/task-config.yaml`:
- Around line 244-248: The apiCall URL for the "update-status" task uses the
singular endpoint "/api/hyperfleet/v1/clusters/{{ .clusterId }}/status" which
does not match the repository's expected plural form; update the URL to
"/api/hyperfleet/v1/clusters/{{ .clusterId }}/statuses" in the "update-status"
apiCall so dry-run mocks and the HyperFleet API use the correct plural endpoint.
---
Nitpick comments:
In `@internal/dryrun/dryrun_responses.go`:
- Around line 45-52: Loop over mrf.Responses in the existing validation loop and
validate each ep.Match.URLPattern by attempting to compile it (e.g., using
regexp.Compile); if compilation fails return a descriptive error including the
pattern and the regex error so the function fails fast on invalid patterns. Keep
this check adjacent to the existing emptiness check for ep.Match.URLPattern in
the same loop that iterates mrf.Responses so malformed regexes are caught early.
In `@test/testdata/dryrun/task-config.yaml`:
- Around line 52-55: The timestamp field currently hardcodes a format string
instead of producing a current timestamp; locate the task entry with name
"timestamp" and replace the expression value so it invokes the runtime's "now"
function (not a quoted format literal) and formats it as an RFC3339 string;
e.g., remove the escaped quotes and use the config's expression syntax to call
now() (or the template engine's {{ now }} equivalent) with an RFC3339 formatter
so the expression returns a concrete timestamp rather than the literal format
string.
ciaranRoche
reviewed
Feb 17, 2026
Contributor
ciaranRoche
left a comment
ciaranRoche
left a comment
There was a problem hiding this comment.
Aside from some of the code rabbit comments, everything looks good to me, tested locally
rh-amarin
force-pushed
the
new-dryrun
branch
from
b52bb5a to
aaa4c7b
Compare
rh-amarin
force-pushed
the
new-dryrun
branch
from
aaa4c7b to
ab18539
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/dryrun/trace.go (1)
129-151: Consider defensive bounds checking for API request index.The
apiReqIdxtracking assumes a strict 1:1 correspondence between preconditions/post-actions withAPICallMade=trueand recorded requests. If this invariant is ever violated (e.g., a request fails before being recorded), the index could become misaligned.The current implementation does check
apiReqIdx < len(t.APIClient.Requests), which prevents panics, but misalignment could show the wrong request for a precondition.Given this is for dry-run mode with controlled mock clients, this is likely acceptable, but consider adding a comment documenting the assumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/dryrun/trace.go` around lines 129 - 151, The loop that prints API calls assumes a 1:1 correspondence between entries with APICallMade=true in result.PreconditionResults (and post-actions) and entries in t.APIClient.Requests, tracked by apiReqIdx; add a concise comment above the apiReqIdx := 0 initialization (and/or above the loop) documenting this invariant and that the existing check apiReqIdx < len(t.APIClient.Requests) is intended to prevent panics if the invariant is violated, and optionally note that a mismatch may cause the wrong request to be shown so callers should ensure mock clients record requests in order (reference symbols: apiReqIdx, result.PreconditionResults, APICallMade, t.APIClient.Requests, prettyJSON).internal/dryrun/discovery_overrides_test.go (1)
50-80: Consider adding a test for malformed JSON.The existing tests cover missing required fields well. For completeness, you might add a test for invalid JSON syntax to verify the parse error path.
💡 Optional test case
func TestLoadDiscoveryOverrides_InvalidJSON(t *testing.T) { t.Run("malformed JSON returns parse error", func(t *testing.T) { dir := t.TempDir() path := writeOverrideFile(t, dir, "overrides.json", `{not valid json}`) _, err := LoadDiscoveryOverrides(path) require.Error(t, err) assert.Contains(t, err.Error(), "failed to parse") }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/dryrun/discovery_overrides_test.go` around lines 50 - 80, Add a unit test that verifies LoadDiscoveryOverrides returns an error when the overrides file contains malformed JSON: create a new test named TestLoadDiscoveryOverrides_InvalidJSON (or similar) that uses t.TempDir and writeOverrideFile to write a deliberately invalid JSON string (e.g. "{not valid json}") to "overrides.json", call LoadDiscoveryOverrides(path), and assert an error is returned and the error message indicates a parse failure (e.g. contains "failed to parse" or similar).cmd/adapter/main.go (1)
572-664: LGTM overall, with one minor observation.The dry-run implementation is well-structured:
- Creates logger on stderr to keep stdout clean for trace output
- Reuses shared config loading and executor building
- Properly loads and validates all dry-run inputs
- Outputs trace in the requested format
One observation: when
result.Status == executor.StatusFailed, errors are printed to stderr (line 658-660) but the function still returnsnil. Consider returning a non-nil error to signal failure to the caller.💡 Optional: Return error on execution failure
if result.Status == executor.StatusFailed { for phase, err := range result.Errors { fmt.Fprintf(os.Stderr, "Error in %s: %v\n", phase, err) } + return fmt.Errorf("dry-run execution failed") } return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/adapter/main.go` around lines 572 - 664, The function runDryRun currently prints execution errors when result.Status == executor.StatusFailed but still returns nil; update runDryRun to return a non-nil error in that branch (e.g., construct and return an error from the result.Errors map or a summary like fmt.Errorf("dry-run execution failed: %v", result.Errors)) so callers can detect failure; modify the block referencing executor.StatusFailed / result.Errors to both write to stderr and then return the constructed error instead of falling through to nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/adapter/main.go`:
- Around line 171-174: The isDryRun detection is too permissive (returns true if
dryRunAPIResponses is set) which triggers dry-run mode without a required
--dry-run-event; update the isDryRun function so it only returns true when
dryRunEvent is non-empty (i.e., require dryRunEvent rather than either flag), by
changing the boolean expression to check only dryRunEvent != "" so downstream
code expecting a dry-run event won't error unexpectedly; keep references to
isDryRun, dryRunEvent and dryRunAPIResponses so reviewers can verify behavior.
---
Nitpick comments:
In `@cmd/adapter/main.go`:
- Around line 572-664: The function runDryRun currently prints execution errors
when result.Status == executor.StatusFailed but still returns nil; update
runDryRun to return a non-nil error in that branch (e.g., construct and return
an error from the result.Errors map or a summary like fmt.Errorf("dry-run
execution failed: %v", result.Errors)) so callers can detect failure; modify the
block referencing executor.StatusFailed / result.Errors to both write to stderr
and then return the constructed error instead of falling through to nil.
In `@internal/dryrun/discovery_overrides_test.go`:
- Around line 50-80: Add a unit test that verifies LoadDiscoveryOverrides
returns an error when the overrides file contains malformed JSON: create a new
test named TestLoadDiscoveryOverrides_InvalidJSON (or similar) that uses
t.TempDir and writeOverrideFile to write a deliberately invalid JSON string
(e.g. "{not valid json}") to "overrides.json", call
LoadDiscoveryOverrides(path), and assert an error is returned and the error
message indicates a parse failure (e.g. contains "failed to parse" or similar).
In `@internal/dryrun/trace.go`:
- Around line 129-151: The loop that prints API calls assumes a 1:1
correspondence between entries with APICallMade=true in
result.PreconditionResults (and post-actions) and entries in
t.APIClient.Requests, tracked by apiReqIdx; add a concise comment above the
apiReqIdx := 0 initialization (and/or above the loop) documenting this invariant
and that the existing check apiReqIdx < len(t.APIClient.Requests) is intended to
prevent panics if the invariant is violated, and optionally note that a mismatch
may cause the wrong request to be shown so callers should ensure mock clients
record requests in order (reference symbols: apiReqIdx,
result.PreconditionResults, APICallMade, t.APIClient.Requests, prettyJSON).
Contributor
|
/lgtm |
ciaranRoche
approved these changes
Feb 18, 2026
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.
https://issues.redhat.com/browse/HYPERFLEET-652
It can be tested with the test files at
test/testdata/dryrunTest plan
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Improvements
Tests