Skip to content

Conversation

@jferrazbr
Copy link

@jferrazbr jferrazbr commented Dec 19, 2025

Issue: #52464

Problem

Rancher Machine drivers define CLI flags using mcnflag. For boolean flags, the current flow effectively assumes the default is always false. This makes it hard to express flags that should default to true, like Azure managed disks, and causes inconsistencies across Rancher components (Rancher UI, terraform provider, and machine).

Azure unmanaged disks are being retired, so we need azure-managed-disks to default to true and still allow users to explicitly disable it.

Solution

Bool flags default support

  • Extend mcnflag.BoolFlag with a Value bool field and return it in BoolFlag.Default().

  • Update CLI flag conversion (convertMcnFlagsToCliFlags) to preserve default true:

    • If mcnflag.BoolFlag.Value is true, convert it to cli.BoolTFlag (default true).
    • Otherwise keep cli.BoolFlag (default false).

Pass default bool values through RPC opts

  • Update libmachine/drivers/rpc/util.go so GetDriverOpts passes a default value to setFlag only when the bool default is true. This keeps existing behavior for default false, and allows default true flags to be represented correctly.

Azure managed disks default to true

  • Set Value: true for azure-managed-disks.
  • Update the flag help text to call out the default and the Azure retirement date for unmanaged disks.

Tests

  • Add a table-driven test for convertMcnFlagsToCliFlags covering:

    • Bool default true -> cli.BoolTFlag
    • Bool default false -> cli.BoolFlag
    • Other flag types and mixed inputs
    • EnvVar preservation
    • Azure managed disks case
  • Extend RPC util tests to cover:

    • bool flags with default true (value and pointer variants)
    • ensuring default true is present in parsed driver opts

Testing

Manual Testing

  • CLI (rancher-machine binary)

    • Created an Azure VM without passing --azure-managed-disks.
    • Verified the resulting VM used managed disks by default.
  • RPC (driver opts via CRD)

    • Manually created an AzureConfig CRD without specifying the managed disks option.
    • Verified the created config defaulted managedDisks to true.
    • Note: this required a small supporting change in rancher/rancher (tracked in a follow-up PR).

Automated Testing

  • Unit tests added/updated:

    • commands/create_test.go (flag conversion coverage)
    • libmachine/drivers/rpc/util_test.go (driver opts default-true coverage)

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for boolean flags with a default value of true in the Rancher Machine driver framework, specifically to make Azure managed disks default to enabled. This change addresses Azure's retirement of unmanaged disks by March 31, 2026.

Key changes:

  • Extended mcnflag.BoolFlag with a Value field to support default true values
  • Updated CLI flag conversion to use BoolTFlag for default-true boolean flags
  • Modified RPC utilities to properly pass boolean default values
  • Set Azure managed disks flag to default to true with updated documentation

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
libmachine/mcnflag/flag.go Added Value field to BoolFlag struct and updated Default() method to return it instead of hardcoded false
libmachine/drivers/rpc/util.go Updated GetDriverOpts to pass boolean default values when Value is true, maintaining backward compatibility for default false
libmachine/drivers/rpc/util_test.go Added test coverage for boolean flags with default true for both pointer and value variants
commands/create.go Modified convertMcnFlagsToCliFlags to convert default-true BoolFlags to cli.BoolTFlag
commands/create_test.go Added comprehensive table-driven tests for flag conversion covering multiple flag types and the Azure managed disks case
drivers/azure/azure.go Set azure-managed-disks flag to default to true and updated usage text to document the default and Azure's retirement date

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

1 participant