pscloud: Release stable version 1.0.0 with CLI improvements #9488
Conversation
❌Azure CLI Extensions Breaking Change Test
|
|
Hi @DhritiJindal27, |
|
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>
|
CodeGen Tools Feedback CollectionThank you for using our CodeGen tool. We value your feedback, and we would like to know how we can improve our product. Please take a few minutes to fill our codegen survey |
Release SuggestionsModule: pscloud
Notes
|
DhritiJindal27
requested review from
bhargavi007,
deepakmauryams and
munishgoyal1
There was a problem hiding this comment.
Pull request overview
This PR releases the pscloud Azure CLI extension as stable version 1.0.0, transitioning from beta (1.0.0b1) to production-ready status. The changes include removing preview flags from all commands, standardizing CLI parameter names for better consistency, simplifying network parameter usage, removing unsupported identity features, and updating documentation with improved examples.
Key Changes:
- Promoted extension to stable version 1.0.0 with dual version management system
- Standardized parameter naming to use
--name/-nacross commands and simplified zone/network parameters - Removed unsupported identity parameters and wait command functionality
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Implements dual version management (STABLE_VERSION/PREVIEW_VERSION) and sets current version to 1.0.0 |
| azext_pscloud/aaz/latest/pscloud/__cmd_group.py | Removes is_preview flag from pscloud command group |
| azext_pscloud/aaz/latest/pscloud/_list.py | Removes is_preview flag from list command |
| azext_pscloud/aaz/latest/pscloud/_show.py | Removes is_preview flag from show command |
| azext_pscloud/aaz/latest/pscloud/pool/__cmd_group.py | Removes is_preview flag from pool command group |
| azext_pscloud/aaz/latest/pscloud/pool/init.py | Removes import of deleted _wait module |
| azext_pscloud/aaz/latest/pscloud/pool/_create.py | Removes identity parameters, flattens network parameters to --subnet-name/--vnet-name, adds --zone parameter with required validation, updates example |
| azext_pscloud/aaz/latest/pscloud/pool/_update.py | Removes is_preview flag and identity-related parameters |
| azext_pscloud/aaz/latest/pscloud/pool/_delete.py | Removes is_preview flag |
| azext_pscloud/aaz/latest/pscloud/pool/_list.py | Removes is_preview flag |
| azext_pscloud/aaz/latest/pscloud/pool/_show.py | Removes is_preview flag |
| azext_pscloud/aaz/latest/pscloud/pool/_get_health_status.py | Removes is_preview flag and standardizes parameter to --name/-n |
| azext_pscloud/aaz/latest/pscloud/pool/_get_avs_status.py | Removes is_preview flag and standardizes parameter to --name/-n |
| azext_pscloud/aaz/latest/pscloud/pool/_wait.py | Completely removes wait command (entire file deleted) |
| README.md | Documents parameter improvements, adds usage examples with realistic Azure resource IDs, removes references to preview and wait command, updates installation instructions |
| properties.set_prop("provisionedBandwidthMbPerSec", AAZIntType, ".provisioned_bandwidth", typ_kwargs={"flags": {"required": True}}) | ||
| properties.set_prop("reservationResourceId", AAZStrType, ".reservation_id", typ_kwargs={"flags": {"required": True}}) | ||
| properties.set_prop("vnetInjection", AAZObjectType, ".vnet_injection", typ_kwargs={"flags": {"required": True}}) | ||
| properties.set_prop("vnetInjection", AAZObjectType, ".", typ_kwargs={"flags": {"required": True}}) |
There was a problem hiding this comment.
The second parameter "." in the vnetInjection set_prop call is likely incorrect. Following the pattern used elsewhere in the codebase (e.g., line 228 for "properties", and similar patterns in other files), when setting an object type that is composed of multiple sub-fields rather than mapping to a single argument, the second parameter should be omitted entirely.
The vnetInjection object is built from subnet_id and vnet_id fields (lines 240-241), so this should probably be:
properties.set_prop("vnetInjection", AAZObjectType, typ_kwargs={"flags": {"required": True}})
Using "." as the second parameter would attempt to use the entire args context as the value, which doesn't make sense given that the actual values are set on lines 240-241.
| properties.set_prop("vnetInjection", AAZObjectType, ".", typ_kwargs={"flags": {"required": True}}) | |
| properties.set_prop("vnetInjection", AAZObjectType, typ_kwargs={"flags": {"required": True}}) |
| ```bash | ||
| az pscloud pool create \ | ||
| --resource-group myResourceGroup \ | ||
| --storage-pool-name myStoragePool \ |
There was a problem hiding this comment.
The example uses the old parameter name --storage-pool-name instead of the standardized --name parameter. For consistency with the PR's goal of parameter standardization and the documented improvements on lines 12 and 154, this should be changed to use --name or -n.
| --storage-pool-name myStoragePool \ | |
| --name myStoragePool \ |
There was a problem hiding this comment.
No changes made in create, changes regarding storage pool name have only been made in get-avs-status and get-health status.
| ) | ||
| _args_schema.storage_pool_name = AAZStrArg( | ||
| options=["--storage-pool-name"], | ||
| options=["-n", "--name"], |
There was a problem hiding this comment.
The parameter options only include ["-n", "--name"] without the "--storage-pool-name" alias. However, other commands in the same command group (_create.py line 52, _show.py line 51, _delete.py line 53, _update.py line 52) include all three options: ["-n", "--name", "--storage-pool-name"].
For consistency and backward compatibility, this should also include "--storage-pool-name" as an alias option, similar to the other commands.
| options=["-n", "--name"], | |
| options=["-n", "--name", "--storage-pool-name"], |
| ) | ||
| _args_schema.storage_pool_name = AAZStrArg( | ||
| options=["--storage-pool-name"], | ||
| options=["-n", "--name"], |
There was a problem hiding this comment.
The parameter options only include ["-n", "--name"] without the "--storage-pool-name" alias. However, other commands in the same command group (_create.py line 52, _show.py line 51, _delete.py line 53, _update.py line 52) include all three options: ["-n", "--name", "--storage-pool-name"].
For consistency and backward compatibility, this should also include "--storage-pool-name" as an alias option, similar to the other commands.
| options=["-n", "--name"], | |
| options=["-n", "--name", "--storage-pool-name"], |
| _args_schema.subnet_id = AAZStrArg( | ||
| options=["--subnet-name"], | ||
| arg_group="VnetInjection", | ||
| help="Azure resource ID of the Virtual Network subnet where the storage pool will be connected", | ||
| required=True, | ||
| ) | ||
| _args_schema.vnet_id = AAZStrArg( | ||
| options=["--vnet-name"], | ||
| arg_group="VnetInjection", | ||
| help="Azure resource ID of the Virtual Network in which the subnet is located", | ||
| required=True, | ||
| ) |
There was a problem hiding this comment.
The parameter option names are misleading and inconsistent with the help text and actual values expected. The parameter options are named --subnet-name and --vnet-name, but the help text describes them as accepting "Azure resource ID" values, and the internal variable names are subnet_id and vnet_id.
The documentation in lines 90-91 and 102-103 of README.md also shows conflicting information - they suggest these should be simple names like "mySubnet" and "myVnet", but line 11 and 19 mention --subnet-id and --vnet-id, and the help text says these should be full Azure resource IDs.
Either:
- Change the option names to
--subnet-idand--vnet-idto match the help text and internal variable names, OR - Change the help text and documentation to clarify these are names (not IDs) and update the internal variable names accordingly
Based on the PR description mentioning "Network parameter simplification: Use --subnet-name and --vnet-name instead of complex JSON objects", it appears the intention is to use names, but the help text needs to be corrected.
src/pscloud/README.md
Outdated
| - **Flattened network parameters**: Use `--subnet-id` and `--vnet-id` directly instead of complex `--vnet-injection` JSON objects | ||
| - **Improved parameter names**: Consistent use of `--name` or `-n` across all commands | ||
|
|
||
| ### Removed Unsupported Features ### | ||
| - **Identity parameters removed**: `--system-assigned`, `--user-assigned`, and related identity options have been removed as they are not supported by the Pure Storage Cloud service | ||
| - **Wait command removed**: `az pscloud pool wait` has been removed for consistency with other Azure CLI extensions | ||
|
|
||
| ### Enhanced Validation ### | ||
| - **Required parameters**: Key parameters like `--zone`, `--provisioned-bandwidth`, `--reservation-id`, `--subnet-id`, and `--vnet-id` are now properly validated as required | ||
| - **Better examples**: All command examples now show realistic Azure resource IDs and cleaner syntax | ||
|
|
There was a problem hiding this comment.
The documentation is inconsistent regarding the subnet and vnet parameter names. Lines 11 and 19 refer to --subnet-id and --vnet-id, while lines 85-117 use --subnet-name and --vnet-name. Additionally, lines 90-91 state these should be "Name of the delegated subnet" and "Name of the virtual network", while the actual code help text (in _create.py lines 110 and 116) describes these as "Azure resource ID" values.
The examples show simple names like "mySubnet" and "myVnet" (lines 102-103), which contradicts the resource ID description. This documentation needs to be consistent with the actual implementation and clearly specify whether these parameters accept simple names or full Azure resource IDs.
| **Required Parameters:** | ||
| - `--zone` or `-z`: Azure Availability Zone (1, 2, or 3) | ||
| - `--subnet-name`: Name of the delegated subnet | ||
| - `--vnet-name`: Name of the virtual network | ||
| - `--provisioned-bandwidth`: Bandwidth in MB/s | ||
| - `--reservation-id`: Azure resource ID of the Pure Storage Cloud reservation | ||
|
|
||
| **Example with realistic values:** | ||
| ```bash | ||
| az pscloud pool create \ | ||
| --resource-group myResourceGroup \ | ||
| --storage-pool-name myStoragePool \ | ||
| --location eastus \ | ||
| --zone 1 \ | ||
| --subnet-name mySubnet \ | ||
| --vnet-name myVnet \ | ||
| --provisioned-bandwidth 100 \ | ||
| --reservation-id /subscriptions/12345678-1234-1234-1234-123456789abc/providers/PureStorage.Block/reservations/myReservation | ||
| ``` |
There was a problem hiding this comment.
The documentation examples show simple names (e.g., "mySubnet", "myVnet") for the --subnet-name and --vnet-name parameters, but the actual code help text in _create.py describes these as accepting "Azure resource ID" values. The description on lines 90-91 also says "Name of the delegated subnet" and "Name of the virtual network", suggesting simple names, not resource IDs.
The examples should either:
- Show full Azure resource IDs if that's what the parameters expect, OR
- The code help text should be updated to indicate these are names (not resource IDs)
This inconsistency will confuse users about what values to provide.
| :example: StoragePools_Create | ||
| az pscloud pool create --resource-group rgpurestorage --storage-pool-name storagePoolname --availability-zone vknyl --vnet-injection "{subnet-id:tnlctolrxdvnkjiphlrdxq,vnet-id:zbumtytyqwewjcyckwqchiypshv}" --provisioned-bandwidth 17 --reservation-id xiowoxnbtcotutcmmrofvgdi --type None --user-assigned-identities "{key4211:{}}" --tags "{key7593:vsyiygyurvwlfaezpuqu}" --location lonlc | ||
| az pscloud pool create --resource-group rgpurestorage --storage-pool-name storagePoolname --zone 1 --subnet-name /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/virtualNetworks/{vnetName}/subnets/{subnetName} --vnet-name /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/virtualNetworks/{vnetName} --provisioned-bandwidth 100 --reservation-id /subscriptions/{subscriptionId}/providers/PureStorage.Block/reservations/{reservationName} --location eastus |
There was a problem hiding this comment.
The example command in the code uses --storage-pool-name parameter, but according to the PR description and changes to other commands (_get_health_status.py, _get_avs_status.py), the consistent naming should use --name or -n instead. While --storage-pool-name is still supported as an alias (line 52), the example should use the preferred standardized form --name for consistency.
| az pscloud pool create --resource-group rgpurestorage --storage-pool-name storagePoolname --zone 1 --subnet-name /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/virtualNetworks/{vnetName}/subnets/{subnetName} --vnet-name /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/virtualNetworks/{vnetName} --provisioned-bandwidth 100 --reservation-id /subscriptions/{subscriptionId}/providers/PureStorage.Block/reservations/{reservationName} --location eastus | |
| az pscloud pool create --resource-group rgpurestorage --name storagePoolname --zone 1 --subnet-name /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/virtualNetworks/{vnetName}/subnets/{subnetName} --vnet-name /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/virtualNetworks/{vnetName} --provisioned-bandwidth 100 --reservation-id /subscriptions/{subscriptionId}/providers/PureStorage.Block/reservations/{reservationName} --location eastus |
There was a problem hiding this comment.
No changes made in create, changes regarding storage pool name have only been made in get-avs-status and get-health status.
|
@microsoft-github-policy-service agree company="Microsoft" |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 9488 in repo Azure/azure-cli-extensions |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
AAZ PR - Azure/aaz#914 |
|
@microsoft-github-policy-service agree company="Microsoft" |
- Rename -n and --name - Add --zone and -z - Flatten --vnet-injection. - Remove unsupported identity options (--system-assigned, --user-assigned) - Make required parameters mandatory with client-side validation - --zone, --provisioned-bandwidth, --reservation-id - --subnet-id, --vnet-id - Remove wait subcommand for consistency with other Azure CLIs - Update examples to show cleaner syntax with individual flags
- Updated version from 1.0.0b1 to stable 1.0.0 with dual version management - Removed preview flags from all commands and command groups - Fixed parameter consistency across commands (--name/-n standardization) - Updated README documentation to reflect all parameter changes and stable version - Added version management with separate STABLE_VERSION and PREVIEW_VERSION variables
- Changed test_pscloud_storagepool_create to use 'dhritijindal' to match VCR recording - Ensured test_pscloud_storagepool_delete uses 'dhritijindal123' to match its VCR recording - All 6 tests now pass successfully
DhritiJindal27
force-pushed
the
pscloud-stable-v1.0.0
branch
from
daedbe1 to
aba11ba
Compare
4 participants
This PR releases the pscloud extension as stable version 1.0.0 with significant CLI usability improvements.
Clean PR: Only 3 commits, 15 files changed (all pscloud-related)
Changes Made
Stable Release 1.0.0
CLI Parameter Improvements
Bug Fixes
Documentation Updates
Testing
Impact
This PR improves the user experience by making the extension stable and production-ready with simplified parameter usage, consistent naming, and better validation.