Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…4/azure-cli-extensions into user/saif/newCommands
|
Validation for Breaking Change Starting...
Thanks for your contribution! |
|
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 refactors the Azure Migrate CLI extension codebase to improve maintainability and adds new replication management commands. The changes reorganize helper functions into a modular structure and introduce commands for listing replications, removing replications, and retrieving job status.
Key Changes:
- Restructured helper modules from flat files into organized subdirectories (
helpers/replication/,helpers/) - Added three new commands:
list,remove, andget-jobfor replication management - Updated parameter names from
resource_group_nametoresource_groupfor consistency - Version bump from 2.0.1b1 to 3.0.0b1
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Version update to 3.0.0b1 |
| linter_exclusions.yml | Added test coverage exclusions for new commands |
| test_migrate_commands.py | Updated import paths and parameter names in tests |
| custom.py | Refactored imports, added new command implementations |
| commands.py | Added new command registrations |
| _params.py | Updated parameter definitions for consistency |
| _help.py | Added help documentation for new commands |
| _get_discovered_server_helpers.py | Added machine_id to server info output |
| HISTORY.rst | Updated release history |
| helpers/replication/* | New modular helper files for replication operations |
| resource_group_name = machine_id_parts[4] | ||
| else: | ||
| raise CLIError(f"Invalid machine ARM ID format: '{machine_id}'") |
There was a problem hiding this comment.
The function returns rg_uri on line 258, but rg_uri is only assigned in the if machine_index: branch (line 122-124). When machine_id is provided directly (the else path starting at line 205), rg_uri is never set before being returned, which will cause an UnboundLocalError.
| machine_id, | ||
| IdFormats.MachineArmIdTemplate): | ||
| raise CLIError( | ||
| f"Invalid -machine_id '{machine_id}'. " |
There was a problem hiding this comment.
The error message uses -machine_id with a hyphen prefix, but the actual parameter name in the code is machine_id without a leading hyphen. For consistency with CLI parameter naming conventions, this should be --machine-id or just machine_id.
| recoveryPoint = ( | ||
| ReplicationPolicyDetails.RecoveryPointHistoryInMinutes.value | ||
| ) | ||
| crashConsistentFreq = ( | ||
| ReplicationPolicyDetails.CrashConsistentFrequencyInMinutes.value | ||
| ) | ||
| appConsistentFreq = ( | ||
| ReplicationPolicyDetails.AppConsistentFrequencyInMinutes.value | ||
| ) |
There was a problem hiding this comment.
Variable names recoveryPoint, crashConsistentFreq, and appConsistentFreq use camelCase, which is inconsistent with Python's PEP 8 naming convention for variables. These should use snake_case: recovery_point, crash_consistent_freq, and app_consistent_freq.
| protected_item_uri, | ||
| APIVersion.Microsoft_DataReplication.value, | ||
| protected_item_body) |
There was a problem hiding this comment.
The protected_item_uri is missing a leading / character. Line 252 constructs it as subscriptions/{subscription_id}/... but ARM resource IDs should start with /subscriptions/.... This will cause API requests to fail.
| print(" az migrate local replication get-job " | ||
| "--job-name {} " | ||
| "--resource-group {} " | ||
| "--project-name <project-name>".format(job_name, resource_group_name)) |
There was a problem hiding this comment.
The print statement uses hardcoded <project-name> placeholder while other values are formatted. Consider passing the actual project name if available, or use a consistent placeholder format for all parameters (e.g., <resource-group>, <job-name>).
| current_tool = amh_solution.get('properties', {}).get('tool') | ||
|
|
||
| needs_update = False |
There was a problem hiding this comment.
The variable current_tool is retrieved but not displayed in the logging statement if it needs update. The log on line 224 shows the target value but not the current value, making it harder to debug tool name mismatches.
2 participants
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json.