OSAC-717: add is_windows field to ComputeInstanceSpec API - #734
openshift-merge-bot[bot] merged 1 commit into
Conversation
|
@ygalblum: This pull request references OSAC-717 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (4)
WalkthroughAdds an Changesis_windows Guest OS Field
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@internal/controllers/computeinstance/computeinstance_reconciler_function_test.go`:
- Line 193: Replace the human-readable string fixture IDs with UUIDv7-formatted
IDs in the computeinstance_reconciler_function_test.go test file. Specifically,
update the assignments for variables like subnetID and any other
ComputeInstance/Subnet fixture ID strings (found at lines 193, 214, 238, 259,
283, and 303) to use valid UUIDv7-formatted strings instead of descriptive names
like "test-subnet". This ensures test fixtures follow the repository convention
for resource ID formatting.
In `@proto/public/osac/public/v1/compute_instance_type.proto`:
- Around line 173-179: In the JSON example within the documentation comment for
the compute instance type proto, the template field value is currently set to a
hardcoded string "123". Replace this with a valid UUIDv7-formatted string to
align with the repository's convention for resource IDs in proto documentation
examples and test fixtures. The change affects only the example value on the
line containing "template": "123".
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: cb73f0d2-2d7e-4c86-a5f0-a1e12ff486c3
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.suminternal/api/osac/private/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
go.modinternal/controllers/computeinstance/computeinstance_reconciler_function.gointernal/controllers/computeinstance/computeinstance_reconciler_function_test.goproto/private/osac/private/v1/compute_instance_type.protoproto/public/osac/public/v1/compute_instance_type.proto
|
/rebase |
a70b337 to
106114a
Compare
Add is_windows optional boolean proto field to ComputeInstanceSpec in both public and private APIs (field number 16). When true, the compute instance is configured for a Windows guest OS; when false or omitted, it defaults to Linux. The reconciler maps this field to the CRD GuestOSFamily string field in addExplicitFields(), ensuring the CR always has an explicit value (windows or linux) for the AAP provisioning layer. Changes: - Add optional bool is_windows = 16 to public and private proto schemas - Regenerate Go code with buf generate (HasIsWindows/GetIsWindows accessors) - Bump osac-operator/api dependency from v0.0.4 to v0.0.5 - Implement is_windows to guestOSFamily mapping in reconciler - Add unit tests covering all three mapping scenarios (true/false/omitted) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
106114a to
2a14cf7
Compare
oourfali
left a comment
oourfali
left a comment
There was a problem hiding this comment.
Approved based on previous approval before rebase.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, oourfali, ygalblum 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 |
5 participants
Add is_windows optional boolean proto field to ComputeInstanceSpec in both public and private APIs (field number 16). When true, the compute instance is configured for a Windows guest OS; when false or omitted, it defaults to Linux.
The reconciler maps this field to the CRD GuestOSFamily string field in addExplicitFields(), ensuring the CR always has an explicit value (windows or linux) for the AAP provisioning layer.
Changes:
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit