[Draft] Support --what-if argument v2#32477
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
wangzelin007
requested review from
calvinhzy,
evelyn-ys,
jsntcy and
yanzhudd
as code owners
️✔️AzureCLI-FullTest
|
wangzelin007
requested review from
bebound,
jiasli,
kairu-ms and
necusjz
as code owners
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| network vnet create | cmd network vnet create added parameter export_bicep |
||
| network vnet create | cmd network vnet create added parameter what_if |
||
| network vnet update | cmd network vnet update added parameter export_bicep |
||
| network vnet update | cmd network vnet update added parameter what_if |
⚠️ storage
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| storage account create | cmd storage account create added parameter export_bicep |
||
| storage account create | cmd storage account create added parameter what_if |
||
| storage account network-rule add | cmd storage account network-rule add added parameter export_bicep |
||
| storage account network-rule add | cmd storage account network-rule add added parameter what_if |
||
| storage container create | cmd storage container create added parameter export_bicep |
||
| storage container create | cmd storage container create added parameter what_if |
||
| storage share create | cmd storage share create added parameter export_bicep |
||
| storage share create | cmd storage share create added parameter what_if |
⚠️ vm
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| vm create | cmd vm create added parameter export_bicep |
||
| vm create | cmd vm create added parameter what_if |
||
| vm disk attach | cmd vm disk attach added parameter export_bicep |
||
| vm disk attach | cmd vm disk attach added parameter what_if |
||
| vm disk detach | cmd vm disk detach added parameter export_bicep |
||
| vm disk detach | cmd vm disk detach added parameter what_if |
||
| vm nic remove | cmd vm nic remove added parameter export_bicep |
||
| vm nic remove | cmd vm nic remove added parameter what_if |
||
| vm update | cmd vm update added parameter export_bicep |
||
| vm update | cmd vm update added parameter what_if |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
There was a problem hiding this comment.
Pull request overview
This PR introduces support for the --what-if and --export-bicep arguments across multiple Azure CLI commands to enable preview of changes before execution. The implementation integrates with an external what-if service to analyze Azure CLI commands and predict their impact on Azure resources.
Key Changes:
- New
what_if.pymodule implementing the what-if analysis functionality with external service integration - Addition of
--what-ifand--export-bicepparameters to VM, storage, and network commands - Update to issue reporting URL in the resource formatter to use a specific GitHub template
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 27 comments.
Show a summary per file
| File | Description |
|---|---|
src/azure-cli-core/azure/cli/core/what_if.py |
New module implementing what-if service integration with progress indicators and Bicep template conversion |
src/azure-cli-core/azure/cli/core/commands/__init__.py |
Integration of what-if logic into command execution flow with supported command whitelist |
src/azure-cli-core/azure/cli/core/commands/parameters.py |
Definition of reusable what_if and export_bicep parameter types |
src/azure-cli/azure/cli/command_modules/vm/custom.py |
Addition of what_if and export_bicep parameters to VM command functions |
src/azure-cli/azure/cli/command_modules/vm/_params.py |
Parameter registration for VM commands including create, update, disk operations, and NIC management |
src/azure-cli/azure/cli/command_modules/storage/operations/fileshare.py |
Addition of what-if parameters to file share creation |
src/azure-cli/azure/cli/command_modules/storage/operations/blob.py |
Addition of what-if parameters to container creation |
src/azure-cli/azure/cli/command_modules/storage/operations/account.py |
Addition of what-if parameters to storage account operations |
src/azure-cli/azure/cli/command_modules/storage/_params.py |
Parameter registration for storage commands |
src/azure-cli/azure/cli/command_modules/network/aaz/latest/network/vnet/_update.py |
AAZ schema definition for vnet update what-if parameters |
src/azure-cli/azure/cli/command_modules/network/aaz/latest/network/vnet/_create.py |
AAZ schema definition for vnet create what-if parameters |
src/azure-cli/azure/cli/command_modules/network/_params.py |
Parameter registration for network vnet commands |
src/azure-cli/azure/cli/command_modules/resource/_formatters.py |
Updated issue reporting URL to use specific GitHub template |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| payload = { | ||
| "azcli_script": azcli_script, | ||
| "export_bicep": export_bicep, | ||
| "subscription_id": subscription_id | ||
| } |
There was a problem hiding this comment.
Potential information disclosure: The payload includes the full Azure CLI script and subscription ID. Ensure this is properly documented and users are aware of what information is being sent to the external service. Consider:
- Sanitizing sensitive information from commands (e.g., passwords, secrets)
- Adding clear documentation about data transmission
- Implementing data retention policies
|
|
||
| # Check if command is in whitelist | ||
| if not self._is_command_supported_for_what_if(args): | ||
| error_msg = ("\"--what-if\" argument is not supported for this command.") |
There was a problem hiding this comment.
The error message format is inconsistent. Using f-string formatting with .format() style placeholders mixed in other parts of the code. This error should follow the same pattern as others:
error_msg = "\"--what-if\" argument is not supported for this command."Or use consistent formatting throughout.
| what_if_type = CLIArgumentType( | ||
| options_list=['--what-if'], | ||
| help="Preview the changes that will be made without actually executing the command. " | ||
| "This will call the what-if service to compare the current state with the expected state after execution.", |
There was a problem hiding this comment.
[nitpick] Documentation issue: The help text doesn't specify the argument type (boolean) or default value. Consider making it more explicit:
help="Preview the changes that will be made without actually executing the command. "
"This will call the what-if service to compare the current state with the expected state after execution. "
"Default: False",| "This will call the what-if service to compare the current state with the expected state after execution.", | |
| "This will call the what-if service to compare the current state with the expected state after execution. " | |
| "Type: boolean. Default: False.", |
| except: | ||
| pass |
There was a problem hiding this comment.
The exception handling is too broad and suppresses all exceptions without logging. This could hide important errors during cleanup. Consider handling specific exception types or at least logging the error.
except Exception as ex:
logger.warning("Failed to clear progress indicator: %s", str(ex))| def detach_managed_data_disk(cmd, resource_group_name, vm_name, disk_name=None, force_detach=None, disk_ids=None, | ||
| what_if=False, export_bicep=False): |
There was a problem hiding this comment.
Unused parameters what_if and export_bicep in the function signature. Either use them or mark as intentionally unused.
| except: | ||
| pass |
There was a problem hiding this comment.
The same bare exception handling issue exists here. Use specific exception types or at least log the error:
except Exception as ex:
logger.warning("Failed to clear progress indicator: %s", str(ex))| exclude_zones=None, align_regional_disks_to_vm_zone=None, wire_server_mode=None, imds_mode=None, | ||
| wire_server_access_control_profile_reference_id=None, imds_access_control_profile_reference_id=None, | ||
| key_incarnation_id=None, add_proxy_agent_extension=None): | ||
| key_incarnation_id=None, add_proxy_agent_extension=None, what_if=False, export_bicep=False): |
There was a problem hiding this comment.
Unused parameters in the function signature should be either removed or explicitly marked with a leading underscore to indicate they're intentionally unused. The what_if and export_bicep parameters are added but never used in the function body.
If these parameters are meant to be intercepted before the function is called, document this behavior or remove them from the signature.
| key_incarnation_id=None, add_proxy_agent_extension=None, what_if=False, export_bicep=False): | |
| key_incarnation_id=None, add_proxy_agent_extension=None): |
|
|
||
| def create_share(cmd, client, metadata=None, quota=None, fail_on_exist=False, timeout=None, **kwargs): | ||
| def create_share(cmd, client, metadata=None, quota=None, fail_on_exist=False, timeout=None, | ||
| what_if=False, export_bicep=False, **kwargs): |
There was a problem hiding this comment.
Unused parameters what_if and export_bicep in the function signature. Either use them or mark as intentionally unused.
| what_if=False, export_bicep=False, **kwargs): | |
| _what_if=False, _export_bicep=False, **kwargs): |
| metadata=None, public_access=None, fail_on_exist=False, timeout=None, | ||
| default_encryption_scope=None, prevent_encryption_scope_override=None): | ||
| default_encryption_scope=None, prevent_encryption_scope_override=None, | ||
| what_if=None, export_bicep=None): |
There was a problem hiding this comment.
Inconsistent parameter types: what_if and export_bicep should use consistent default values. In this file they default to None, while in other files they default to False. This inconsistency could lead to unexpected behavior when these parameters are checked.
Recommend using False as the default consistently across all functions.
| what_if=None, export_bicep=None): | |
| what_if=False, export_bicep=False): |
|
|
||
| def add_network_rule(cmd, client, resource_group_name, account_name, action='Allow', subnet=None, | ||
| vnet_name=None, ip_address=None, tenant_id=None, resource_id=None): # pylint: disable=unused-argument | ||
| vnet_name=None, ip_address=None, tenant_id=None, resource_id=None, |
There was a problem hiding this comment.
Trailing whitespace on this line should be removed.
| vnet_name=None, ip_address=None, tenant_id=None, resource_id=None, | |
| vnet_name=None, ip_address=None, tenant_id=None, resource_id=None, |
Related command
Description
Testing Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.