Backfill v2 changes #9443
Backfill v2 changes #9443
Conversation
|
Validation for Breaking Change Starting...
Thanks for your contribution! |
|
Hi @guptahars, |
|
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 |
|
Hi @guptahars Release SuggestionsModule: workload-orchestration
Notes
|
There was a problem hiding this comment.
Pull request overview
This PR introduces "Backfill v2 changes" for the workload-orchestration Azure CLI extension, adding new commands and updating existing ones to support configuration schema operations, template hierarchy management, and improved configuration workflows.
Key Changes:
- Added new command groups for configuration schema management and config-template hierarchy operations
- Introduced a
ConfigurationHelperutility class to handle common operations like retrieving configuration IDs, template identifiers, and schemas - Refactored configuration commands (
show,set,download) to use dynamic configuration paths based on hierarchy IDs instead of hardcoded target names
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
configuration/schema/__cmd_group.py |
New command group for managing configuration template schemas |
configuration/_schema_show.py |
New command to display schema placeholders for configuration and solution templates |
configuration/_config_helper.py |
New helper class providing utility methods for configuration operations (ID resolution, schema retrieval, template lookups) |
configuration/_config_show.py |
Updated to use hierarchy IDs and dynamic configuration paths instead of static naming |
configuration/_config_set.py |
Enhanced with YAML validation, create/update flow handling, and schema-based placeholder generation |
configuration/_config_download.py |
Refactored to use dynamic configuration names and improved filename generation |
configuration/__init__.py |
Added import for new schema_show module |
config_template/hierarchy/__cmd_group.py |
New command group for config template hierarchy management |
config_template/hierarchy/_show.py |
New command to show linked hierarchies for config templates |
config_template/hierarchy/__init__.py |
Module initialization for hierarchy subcommands |
config_template/_link.py |
New command to link config templates to hierarchies |
config_template/_unlink.py |
New command to unlink config templates from hierarchies |
config_template/__init__.py |
Added imports for new link/unlink commands and hierarchy submodule |
| for linked_hierarchy in linked: | ||
| context_info["linkedHierarchies"].extend( | ||
| linked_hierarchy.get("hierarchyIds", []) | ||
| ) |
There was a problem hiding this comment.
Potential performance issue: The nested loop starting at line 89 uses .extend() inside a loop which builds up the linkedHierarchies list. If there are many linked_hierarchy items, this could be inefficient. Consider using a list comprehension or flattening the structure in a single pass.
| for linked_hierarchy in linked: | |
| context_info["linkedHierarchies"].extend( | |
| linked_hierarchy.get("hierarchyIds", []) | |
| ) | |
| context_info["linkedHierarchies"] = [ | |
| hid | |
| for linked_hierarchy in linked | |
| for hid in linked_hierarchy.get("hierarchyIds", []) | |
| ] |
| template_subscription = self.ctx.args.template_subscription if self.ctx.args.template_subscription else self.ctx.subscription_id | ||
| solution_flag = self.ctx.args.solution if self.ctx.args.solution else False |
There was a problem hiding this comment.
Code duplication: The logic for determining template_subscription and solution_flag (lines 134-135) is duplicated in multiple places throughout the file (lines 159-160, etc.). This pattern appears in _config_show.py, _config_set.py, and _config_download.py. Consider extracting this into a helper method to reduce duplication and improve maintainability.
| def __init__(self): | ||
| """Initialize the configuration helper""" | ||
| pass | ||
|
|
There was a problem hiding this comment.
Missing documentation: The __init__ method creates an unused instance. Since all methods are static, this class doesn't need an __init__ method at all, or the class should be documented to clarify that it's meant to be used as a namespace for static helper methods only.
| def __init__(self): | |
| """Initialize the configuration helper""" | |
| pass | |
| # Convert hierarchy_id to string if it's an AAZ type | ||
| hierarchy_id_str = str(hierarchy_id) if hierarchy_id else "" | ||
|
|
||
| def try_get_config_id(lookup_id, api_version = "2025-08-01"): |
There was a problem hiding this comment.
The variable api_version parameter has a default value but is used in a function signature with a space before the equals sign. This should be api_version="2025-08-01" without spaces to follow Python conventions.
| def try_get_config_id(lookup_id, api_version = "2025-08-01"): | |
| def try_get_config_id(lookup_id, api_version="2025-08-01"): |
|
|
||
| yaml.add_representer(type(None), represent_none) | ||
| placeholder_yaml = yaml.dump(placeholder_dict, default_flow_style=False, allow_unicode=True) |
There was a problem hiding this comment.
The YAML representer function is being added to the global YAML module configuration inside a method. This modifies global state and can cause issues if the method is called multiple times or if other code uses YAML differently. Consider using yaml.SafeDumper with a custom representer instance or resetting the representer after use.
| yaml.add_representer(type(None), represent_none) | |
| placeholder_yaml = yaml.dump(placeholder_dict, default_flow_style=False, allow_unicode=True) | |
| class CustomDumper(yaml.SafeDumper): | |
| pass | |
| CustomDumper.add_representer(type(None), represent_none) | |
| placeholder_yaml = yaml.dump( | |
| placeholder_dict, | |
| default_flow_style=False, | |
| allow_unicode=True, | |
| Dumper=CustomDumper | |
| ) |
| placeholder_yaml = yaml.dump(placeholder_dict, default_flow_style=False, allow_unicode=True) | ||
| return placeholder_yaml | ||
| else: | ||
| raise CLIInternalError("No editable configs.") |
There was a problem hiding this comment.
Error message duplication and inconsistency: The same "No editable configs." error message appears at line 421 and line 454. Additionally, line 232 in _config_helper.py uses "No Editable configs." with different capitalization. These should be consolidated to a single consistent message, preferably as a constant or coming from the helper function.
| response = client.send_request(request=request, stream=False) | ||
|
|
||
| if response.http_response.status_code == 404: | ||
| raise CLIInternalError(f"No Editable configs. Schema doesn't exist for template: {template_version_id}") |
There was a problem hiding this comment.
Inconsistent capitalization in error message: "No Editable configs." has capital 'E' while the same message elsewhere uses lowercase. This should be consistent with other error messages, preferably "No editable configs." or better yet, a more descriptive message.
| raise CLIInternalError(f"No Editable configs. Schema doesn't exist for template: {template_version_id}") | |
| raise CLIInternalError(f"No editable configs. Schema doesn't exist for template: {template_version_id}") |
| # Validate that --solution flag is only used with target hierarchy IDs | ||
| if hasattr(self.ctx.args, 'solution') and self.ctx.args.solution: | ||
| hierarchy_id = str(self.ctx.args.hierarchy_id).lower() | ||
| if "microsoft.edge/targets" not in hierarchy_id: | ||
| raise CLIInternalError("The --solution flag can only be used when the hierarchy-id is for a target (Microsoft.Edge/targets). Solutions are only configurable at a target level.") |
There was a problem hiding this comment.
Potential security issue: The validation check for the --solution flag only checks if the string "microsoft.edge/targets" is in the lowercase hierarchy_id. This check could produce false positives if the hierarchy ID contains this substring in an unexpected position (e.g., in a resource group name). Consider using a more robust ARM ID parsing approach to verify the resource type.
| def _output(self, *args, **kwargs): | ||
| result = self.deserialize_output(self.ctx.vars.instance, client_flatten=True) | ||
| print(result["properties"]["values"]) | ||
| pass |
There was a problem hiding this comment.
Unnecessary pass statement after returning a value. This line serves no purpose and should be removed.
| pass |
| except Exception: | ||
| pass | ||
| try: | ||
| configuration_id = try_get_config_id(service_group_id) | ||
| if configuration_id: | ||
| return configuration_id | ||
| except Exception: | ||
| pass | ||
| else: | ||
| try: | ||
| configuration_id = try_get_config_id(hierarchy_id_str) | ||
| if configuration_id: | ||
| return configuration_id | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Exception handling anti-pattern: Catching all exceptions with bare except Exception: and silently passing (lines 100-101, 106-107, 113-114) can hide real errors and make debugging difficult. At minimum, these should log the exception or provide more specific error handling for expected failure cases.
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.