This repository was archived by the owner on Sep 9, 2026. It is now read-only.
Repository navigation
Add compute instance scenario, mock server, and e2e test - #385
Merged
openshift-merge-bot[bot] merged 5 commits intoApr 15, 2026
Merged
openshift-merge-bot[bot] merged 5 commits into
openshift-merge-bot[bot] merged 5 commits into
Conversation
Port from archived fulfillment-cli repo (PR osac-project#100): - Add ComputeInstanceScenario with YAML loader for test data - Add MockComputeInstancesServer and MockComputeInstanceTemplatesServer - Add e2e test validating full CRUD lifecycle (list templates, list/create/get/delete instances) Generated with [Claude Code](https://claude.com/claude-code)
Standalone gRPC server that registers mock compute instance and template servers with scenario data, enabling manual CLI lifecycle testing. Generated with [Claude Code](https://claude.com/claude-code)
WalkthroughAdds testing infrastructure for compute instances: a new gRPC test-server CLI at Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (2)
internal/testing/compute_instance_scenario.go (1)
79-82: Use strict YAML decoding to catch misspelled fields early.
yaml.Unmarshalignores unknown keys, so fixture typos can slip through silently.♻️ Proposed fix
import ( + "bytes" "fmt" "os" @@ var file computeInstanceScenarioFile - if err := yaml.Unmarshal(data, &file); err != nil { + decoder := yaml.NewDecoder(bytes.NewReader(data)) + decoder.KnownFields(true) + if err := decoder.Decode(&file); err != nil { return nil, fmt.Errorf("failed to parse scenario YAML: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testing/compute_instance_scenario.go` around lines 79 - 82, Replace the permissive yaml.Unmarshal call with a strict decoder so unknown/misspelled fields fail fast: create a yaml.NewDecoder over the input data, call decoder.KnownFields(true), then Decode into the computeInstanceScenarioFile variable (file) and propagate the returned error; update the error message to reflect strict YAML decoding failure where the function currently uses yaml.Unmarshal and the computeInstanceScenarioFile type is being populated.cmd/test-server/main.go (1)
31-37: Make the default scenario path resilient for standalone execution.Using a repo-relative default path is brittle when running the compiled binary from another working directory.
♻️ Proposed fix
import ( "flag" "fmt" "log" "net" + "path/filepath" + "runtime" @@ const ( serverPort = "8080" - defaultCIScenarioFile = "internal/testing/testdata/compute-instance-scenario.yaml" ) + +func defaultScenarioPath() string { + _, thisFile, _, ok := runtime.Caller(0) + if !ok { + return "internal/testing/testdata/compute-instance-scenario.yaml" + } + return filepath.Clean(filepath.Join(filepath.Dir(thisFile), "..", "..", "internal", "testing", "testdata", "compute-instance-scenario.yaml")) +} func main() { - ciScenarioFile := flag.String("ci-scenario", defaultCIScenarioFile, "Path to compute instance scenario YAML file") + ciScenarioFile := flag.String("ci-scenario", defaultScenarioPath(), "Path to compute instance scenario YAML file")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/test-server/main.go` around lines 31 - 37, The defaultCIScenarioFile constant is repo-relative and brittle when the binary is run from another CWD; update main to compute the default at runtime: in main (where ciScenarioFile is created) determine the executable's directory via os.Executable(), resolve the default path with filepath.Join(execDir, "internal/testing/testdata/compute-instance-scenario.yaml"), and use that as the default for the -ci-scenario flag (falling back to the existing repo-relative constant if os.Executable() fails); update references to defaultCIScenarioFile/serverPort only as needed.
🤖 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/cmd/cli/create/computeinstance/computeinstance_e2e_test.go`:
- Around line 149-153: The test currently checks only that an error occurred
after calling instanceClient.Get(ctx, &publicv1.ComputeInstancesGetRequest{Id:
createdID}), which can hide unrelated failures; replace the generic
Expect(err).To(HaveOccurred()) with an assertion that the gRPC status code is
codes.NotFound by extracting the status from err (using status.FromError or
status.Code) and comparing to grpc codes.NotFound; update imports to include
"google.golang.org/grpc/status" and "google.golang.org/grpc/codes" and assert
the specific NotFound code for the Get call on createdID.
In `@internal/testing/compute_instance_scenario.go`:
- Around line 84-85: file.toScenario() (and any helper used in the 87-116 block
that maps the `state` string to the proto enum) currently lets unknown state
strings fall through to COMPUTE_INSTANCE_STATE_UNSPECIFIED; change the
conversion so you validate the input string against the enum's allowed values
and return a clear error instead of returning an unspecified value. Locate the
mapping logic inside toScenario() (or the helper that maps `state` strings to
the ComputeInstance_State enum) and replace the blind conversion with a lookup
that fails on unknown keys (use the proto enum name/value map or a switch that
returns an error for default cases) so invalid scenario data is surfaced rather
than masked.
In `@internal/testing/mock_compute_instances_server.go`:
- Around line 159-163: The current substring filter on templateData.ID and
templateData.Name is too loose; change the logic in the template-listing code
that applies the "filter" (where templateData.ID and templateData.Name are
checked) to require exact matches instead of substrings: treat filter as either
the full ID or the full name (use strings.EqualFold for name to be
case-insensitive) and skip the template only when neither exact ID nor exact
name matches; optionally support simple "id:<value>" or "name:<value>" prefixes
by parsing the filter before comparison if you want explicit field scoping.
- Around line 53-79: In Create on MockComputeInstancesServer validate that the
incoming instance has a non-nil Spec and Spec.Template with a non-empty Id and
that the Template Id exists in the mock's known templates (e.g., check
s.templates or whatever map stores valid templates); if missing or unknown
return status.Error(codes.InvalidArgument, "spec.template is required/unknown
template id") so tests fail fast and mock behavior matches real validation.
Ensure this check happens before assigning IDs/state and storing in s.instances.
---
Nitpick comments:
In `@cmd/test-server/main.go`:
- Around line 31-37: The defaultCIScenarioFile constant is repo-relative and
brittle when the binary is run from another CWD; update main to compute the
default at runtime: in main (where ciScenarioFile is created) determine the
executable's directory via os.Executable(), resolve the default path with
filepath.Join(execDir,
"internal/testing/testdata/compute-instance-scenario.yaml"), and use that as the
default for the -ci-scenario flag (falling back to the existing repo-relative
constant if os.Executable() fails); update references to
defaultCIScenarioFile/serverPort only as needed.
In `@internal/testing/compute_instance_scenario.go`:
- Around line 79-82: Replace the permissive yaml.Unmarshal call with a strict
decoder so unknown/misspelled fields fail fast: create a yaml.NewDecoder over
the input data, call decoder.KnownFields(true), then Decode into the
computeInstanceScenarioFile variable (file) and propagate the returned error;
update the error message to reflect strict YAML decoding failure where the
function currently uses yaml.Unmarshal and the computeInstanceScenarioFile type
is being populated.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67ce3abe-6822-4498-ade8-35e0fe9f2a66
📒 Files selected for processing (6)
cmd/test-server/main.gointernal/cmd/cli/create/computeinstance/computeinstance_e2e_test.gointernal/cmd/cli/create/computeinstance/computeinstance_suite_test.gointernal/testing/compute_instance_scenario.gointernal/testing/mock_compute_instances_server.gointernal/testing/testdata/compute-instance-scenario.yaml
jhernand
approved these changes
Apr 9, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, tzvatot The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Generated with [Claude Code](https://claude.com/claude-code)
|
New changes are detected. LGTM label has been removed. |
- Assert NotFound status code in e2e test after delete - Validate enum strings in scenario loader, return error on unknown state - Use quoted literal matching in template filter to avoid partial matches Generated with [Claude Code](https://claude.com/claude-code)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/testing/compute_instance_scenario.go`:
- Around line 96-103: The loop over f.Templates (and similarly f.Instances)
dereferences potential nil YAML entries causing a panic; add a nil-check inside
the loops (for i, t := range f.Templates and for i, inst := range f.Instances)
and if an element is nil return a descriptive error (e.g. "nil template entry at
index %d" or "nil instance entry at index %d") instead of assigning into
scenario.Templates or scenario.Instances; perform these checks before
constructing TemplateData/InstanceData to ensure safe dereference and consistent
structured error handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8871c59b-e739-4594-b682-4cf4c5bc42f5
📒 Files selected for processing (3)
internal/cmd/cli/create/computeinstance/computeinstance_e2e_test.gointernal/testing/compute_instance_scenario.gointernal/testing/mock_compute_instances_server.go
✅ Files skipped from review due to trivial changes (1)
- internal/cmd/cli/create/computeinstance/computeinstance_e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/testing/mock_compute_instances_server.go
The mock server now validates that the template ID exists in the scenario's known templates before accepting a create request, matching real server behavior. Generated with [Claude Code](https://claude.com/claude-code)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/testing/mock_compute_instances_server.go (1)
109-116: Consider deterministic ordering for instanceListoutput.Map iteration order is random, so response order may vary run-to-run. Sorting before returning helps keep CLI/e2e assertions stable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testing/mock_compute_instances_server.go` around lines 109 - 116, The List method on MockComputeInstancesServer builds a slice from the s.instances map which iterates in random order; make the response deterministic by sorting the instances slice before returning (e.g., use sort.Slice on the local instances slice and compare a stable key such as publicv1.ComputeInstance.GetId() or .GetName()). Update the List function to import "sort" if needed and perform the sort on instances after the append loop so CLI/e2e assertions are stable.
🤖 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/testing/mock_compute_instances_server.go`:
- Around line 37-47: The constructor NewMockComputeInstancesServer dereferences
scenario.Instances unconditionally which panics if scenario is nil; update
NewMockComputeInstancesServer to guard against a nil scenario by checking if
scenario == nil and either initialize scenario = &ComputeInstanceScenario{} or
return a server with an empty instances map, then safely iterate over
scenario.Instances (nil-safe) to pre-populate server.instances; refer to
NewMockComputeInstancesServer and ComputeInstanceScenario and the
scenario.Instances loop when applying the change.
- Around line 75-90: The Create handler currently always writes to
s.instances[instance.Id] which silently overwrites on ID collision; update the
logic in Create (mock server method that manipulates s.nextID and s.instances)
to first check whether an entry exists for instance.Id and if so return an error
(e.g., AlreadyExists) instead of replacing it; if using auto-generated IDs
(s.nextID path), loop/generate until a non-colliding ID is found (or fail after
N attempts) before assigning instance.Id and storing into s.instances to avoid
races/overwrites.
---
Nitpick comments:
In `@internal/testing/mock_compute_instances_server.go`:
- Around line 109-116: The List method on MockComputeInstancesServer builds a
slice from the s.instances map which iterates in random order; make the response
deterministic by sorting the instances slice before returning (e.g., use
sort.Slice on the local instances slice and compare a stable key such as
publicv1.ComputeInstance.GetId() or .GetName()). Update the List function to
import "sort" if needed and perform the sort on instances after the append loop
so CLI/e2e assertions are stable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16ba1836-ea3f-4fd8-b519-379e3fe9f301
📒 Files selected for processing (1)
internal/testing/mock_compute_instances_server.go
3 tasks
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
cmd/test-server) for manual CLI testingPorted from archived fulfillment-cli repo (PR #100).
Test plan
ginkgo run -r internal)Generated with Claude Code
Summary by CodeRabbit
Tests
Chores