NO-ISSUE: add is_windows field to ComputeInstanceTemplate and CatalogItem support - #750
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
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 (5)
WalkthroughAdds an optional Changesis_windows default field end-to-end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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}
|
💀 CI Triage: Root cause: The main branch is broken due to a database migration sequence number collision between PR 706 and PR 740, which both added a migration with sequence number 60. Explanation: The boot step failed because the fulfillment-console-proxy deployment timed out during rollout. The proxy depends on the fulfillment-grpc-server, which was crashing at startup. The grpc-server pod logs show it failed to initialize the database with the error 'failed to init driver with path migrations: duplicate migration file: 60_create_external_ip_tables.up.sql'. A search of recently merged PRs reveals that PR 706 added '60_add_projects_immutable_trigger.up.sql' and PR 740 added '60_create_external_ip_tables.up.sql'. Because PR 740 was merged after PR 706 without rebasing, the main branch now contains two migrations with the '60_' prefix, causing the golang-migrate library to fail. This PR (750) does not touch migrations and is failing because it was tested against the broken main branch. Evidence: [
Suggestion: Create a PR to fix the main branch by renaming the migration files from '60_create_external_ip_tables' to '61_create_external_ip_tables'. Once merged, rebase this PR and retrigger the tests. Prow job | Build For deeper investigation, use the |
e6fb3d1 to
979d866
Compare
ygalblum
changed the title
|
@ygalblum: This pull request explicitly references no jira issue. 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. |
… support Add `is_windows` optional boolean field to `ComputeInstanceTemplateSpecDefaults` in both public and private proto APIs, with template default application and CatalogItem field definition support. Windows VM provisioning requires templates to carry a guest OS indicator so that downstream provisioning (osac-aap) can apply Windows-specific configuration. This extends the template defaults mechanism to propagate `is_windows` through the same path as other spec fields like cores, memory, and run_strategy. - Added `optional bool is_windows` field (number 6) to `ComputeInstanceTemplateSpecDefaults` in both public and private proto definitions - Regenerated protobuf Go code for the new field - Extended `ApplySpecDefaults` to propagate the `is_windows` template default when the user has not set it - Added `is_windows` as a recognized path in CatalogItem field definition application (`applyFieldDefinitions`) - Added unit tests for template default application: applies default, preserves user-provided value, handles missing default - Added unit tests for CatalogItem field definitions: applies editable default, forces non-editable value Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
979d866 to
3158f36
Compare
|
💀 CI Triage: Root cause: Helm upgrade fails during the boot step because osac-installer PR 328 changed the OSAC_AAP_TOKEN env var from value to valueFrom, causing a strategic merge patch conflict with the existing snapshot deployment. Explanation: The CI boot step uses a pre-built snapshot flavor ( Evidence: Suggestion: Update the Prow job | Build For deeper investigation, use the |
|
/retest |
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
The implementation mechanics (default propagation, field definitions, tests) are well done and follow the existing patterns. But the field type is a fundamental API design issue that should be fixed before merge.
| Category | Count |
|---|---|
| 🔴 Critical | 1 |
| 💡 Suggestion | 1 |
💡 Suggestion: Even setting the type aside, the field name is_windows bakes a specific value into the schema. A name like guest_os or os_family describes the dimension, not one point on it. guest_os: "windows" reads as a choice; is_windows: true reads as an assertion about one specific OS.
|
/lgtm |
4 participants
Add
is_windowsoptional boolean field toComputeInstanceTemplateSpecDefaultsin both public and private proto APIs, with template default application and CatalogItem field definition support.Windows VM provisioning requires templates to carry a guest OS indicator so that downstream provisioning (osac-aap) can apply Windows-specific configuration. This extends the template defaults mechanism to propagate
is_windowsthrough the same path as other spec fields like cores, memory, and run_strategy.Added
optional bool is_windowsfield (number 6) toComputeInstanceTemplateSpecDefaultsin both public and private proto definitionsRegenerated protobuf Go code for the new field
Extended
ApplySpecDefaultsto propagate theis_windowstemplate default when the user has not set itAdded
is_windowsas a recognized path in CatalogItem field definition application (applyFieldDefinitions)Added unit tests for template default application: applies default, preserves user-provided value, handles missing default
Added unit tests for CatalogItem field definitions: applies editable default, forces non-editable value
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
is_windowssetting to choose the VM operating system (Windows whentrue, Linux whenfalseor omitted). New instances inherit this value automatically unless explicitly overridden.is_windowsis applied correctly from template defaults when the user does not set it.