Skip to content

Add image only if its a boot disk#1471

Open
ngopalak-redhat wants to merge 1 commit intoopenshift:mainfrom
ngopalak-redhat:ngopalak/fix_gcp_boot_disk
Open

Add image only if its a boot disk#1471
ngopalak-redhat wants to merge 1 commit intoopenshift:mainfrom
ngopalak-redhat:ngopalak/fix_gcp_boot_disk

Conversation

@ngopalak-redhat
Copy link

@ngopalak-redhat ngopalak-redhat commented Mar 6, 2026

Context: We are trying to add additional disk to GCP to test split disk: https://issues.redhat.com/browse/OCPSTRAT-188

The fix ensures that image is added only for boot disk automatically. If its a non-boot disk, the image is not automatically added.

Related PR: openshift/machine-api-provider-gcp#141

Summary by CodeRabbit

  • Bug Fixes

    • Automatic disk image assignment now applies only to boot disks; non-boot disks with no image are left unchanged to avoid unintended defaults.
  • Tests

    • Added a test ensuring non-boot disks without images remain empty and that defaulting only affects boot disks, preventing regressions and validating expected behavior.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 6, 2026
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 6, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai
Copy link

coderabbitai bot commented Mar 6, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 53cd58fe-e67a-4193-a97a-f1df0afe474e

📥 Commits

Reviewing files that changed from the base of the PR and between c87a60a and 211fee1.

📒 Files selected for processing (2)
  • pkg/webhooks/machine_webhook.go
  • pkg/webhooks/machine_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/webhooks/machine_webhook_test.go
  • pkg/webhooks/machine_webhook.go

Walkthrough

The defaultGCPDisks logic in the machine webhook now only sets a default disk image when a disk is a boot disk (Boot == true) and Image is empty; non-boot disks with an empty Image are left unchanged. A new unit test covers the non-boot case.

Changes

Cohort / File(s) Summary
Boot Disk Defaulting
pkg/webhooks/machine_webhook.go
Changed defaultGCPDisks so the default image is applied only for disks where Boot == true and Image is empty.
Tests — non-boot disk behavior
pkg/webhooks/machine_webhook_test.go
Adjusted TestDefaultGCPProviderSpec expected values and added test case "non-boot disk without image remains empty" to verify non-boot disks with empty Image are not assigned the default image.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: conditionally applying default disk images only to boot disks instead of all disks with empty images.
Stable And Deterministic Test Names ✅ Passed Pull request uses standard Go testing with table-driven tests via t.Run(), not Ginkgo. All test case names are static and descriptive with no dynamic information.
Test Structure And Quality ✅ Passed The test case follows established patterns, has single responsibility, and demonstrates good quality practices without requiring timeout handling.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 6, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign chrischdi for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ngopalak-redhat
Copy link
Author

/test all

@ngopalak-redhat ngopalak-redhat force-pushed the ngopalak/fix_gcp_boot_disk branch from dd2bfc2 to c87a60a Compare March 6, 2026 08:30
@ngopalak-redhat ngopalak-redhat force-pushed the ngopalak/fix_gcp_boot_disk branch from c87a60a to 211fee1 Compare March 6, 2026 08:38
@ngopalak-redhat
Copy link
Author

/test all

@ngopalak-redhat
Copy link
Author

/test unit

@ngopalak-redhat ngopalak-redhat marked this pull request as ready for review March 6, 2026 09:28
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 6, 2026
@ngopalak-redhat
Copy link
Author

/assign @theobarberbany

@lunarwhite
Copy link
Member

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 6, 2026

@ngopalak-redhat: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants