Conversation
RichardChen820
requested review from
jsntcy,
yanzhudd and
zhoxing-ms
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @RichardChen820, |
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| appconfig kv import | cmd appconfig kv import added parameter aks_cluster |
||
| appconfig kv import | cmd appconfig kv import added parameter configmap_name |
||
| appconfig kv import | cmd appconfig kv import added parameter configmap_namespace |
||
| appconfig kv import | cmd appconfig kv import update parameter source: updated property choices from ['appconfig', 'appservice', 'file'] to ['aks', 'appconfig', 'appservice', 'file'] |
|
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>
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
requested review from
FumingZhang,
avanigupta and
yonzhan
microsoft-github-policy-service
bot
added
Auto-Assign
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for importing key-values from Azure Kubernetes Service (AKS) ConfigMaps to Azure App Configuration, providing a migration path for users wanting to centralize configuration management using App Configuration in AKS environments.
Key changes:
- New import source 'aks' that retrieves ConfigMap data via AKS RunCommand API
- Support for structured data formats (JSON, YAML, properties) with configurable flattening
- Comprehensive parameter validation and error handling for AKS-specific arguments
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
keyvalue.py |
Adds AKS-specific parameters and integration with new ConfigMap reader function |
_params.py |
Defines CLI parameters for AKS cluster, ConfigMap name, and namespace arguments |
_validators.py |
Implements validation logic for AKS cluster identifiers and required parameters |
_kv_import_helpers.py |
Core implementation for reading ConfigMap data via AKS RunCommand and data processing |
_diff_utils.py |
Extends serializer support to include AKS as a valid source mode |
_constants.py |
Adds AKS to the comparison fields mapping for import operations |
test_appconfig_kv_import_export_commands.py |
Integration tests covering ConfigMap import scenarios and error handling |
Comments suppressed due to low confidence (1)
src/azure-cli/azure/cli/command_modules/appconfig/tests/latest/test_appconfig_kv_import_export_commands.py:223
- Commented out try statement should be removed as it serves no purpose and creates confusion about the intended error handling structure.
'label': 'KeyValuesWithFeatures',
| c.argument('appservice_account', validator=validate_appservice_name_or_id, help='ARM ID for AppService OR the name of the AppService, assuming it is in the same subscription and resource group as the App Configuration store. Required for AppService arguments') | ||
|
|
||
| with self.argument_context('appconfig kv import', arg_group='AKS') as c: | ||
| c.argument('aks_cluster', validator=validate_aks_cluster_name_or_id, help='ARM ID for AKS OR the name of the AKS, assuming it is in the same subscription and resource group as the App Configuration store. Required for AKS arguments'), |
There was a problem hiding this comment.
The help message should start with an active voice verb instead of a noun. Consider changing 'ARM ID for AKS...' to 'Specify ARM ID for AKS...' or 'Provide ARM ID for AKS...'
|
|
||
| with self.argument_context('appconfig kv import', arg_group='AKS') as c: | ||
| c.argument('aks_cluster', validator=validate_aks_cluster_name_or_id, help='ARM ID for AKS OR the name of the AKS, assuming it is in the same subscription and resource group as the App Configuration store. Required for AKS arguments'), | ||
| c.argument('configmap_name', help='Name of the ConfigMap. Required for AKS arguments.') |
There was a problem hiding this comment.
The help message should start with an active voice verb instead of a noun. Consider changing 'Name of the ConfigMap...' to 'Specify name of the ConfigMap...' or 'Provide name of the ConfigMap...'
| with self.argument_context('appconfig kv import', arg_group='AKS') as c: | ||
| c.argument('aks_cluster', validator=validate_aks_cluster_name_or_id, help='ARM ID for AKS OR the name of the AKS, assuming it is in the same subscription and resource group as the App Configuration store. Required for AKS arguments'), | ||
| c.argument('configmap_name', help='Name of the ConfigMap. Required for AKS arguments.') | ||
| c.argument('configmap_namespace', help='Namespace of the ConfigMap. default to "default" namespace if not specified.') |
There was a problem hiding this comment.
The help message should start with an active voice verb instead of a noun. Consider changing 'Namespace of the ConfigMap...' to 'Specify namespace of the ConfigMap...' or 'Provide namespace of the ConfigMap...'
| key_values = [] | ||
| for k, v in flattened_data.items(): | ||
| if validate_import_key(key=k): | ||
| key_values.append(KeyValue(key=k, value=v)) | ||
| return key_values |
There was a problem hiding this comment.
This line resets the key_values list after accumulating values from previous iterations, causing data loss. This should either append to the existing list or be moved outside the loop.
| key_values = [] | |
| for k, v in flattened_data.items(): | |
| if validate_import_key(key=k): | |
| key_values.append(KeyValue(key=k, value=v)) | |
| return key_values | |
| for k, v in flattened_data.items(): | |
| if validate_import_key(key=k): | |
| key_values.append(KeyValue(key=k, value=v)) |
There was a problem hiding this comment.
I agree with this comment . Moreover, should the return key_values line be unindented?
There was a problem hiding this comment.
I agree, updated.
RichardChen820
force-pushed
the
appconfig/import/configmap
branch
from
c571a87 to
0816ce3
Compare
RichardChen820
force-pushed
the
appconfig/import/configmap
branch
from
f2dc08d to
a88288e
Compare
RichardChen820
force-pushed
the
appconfig/import/configmap
branch
from
d06d232 to
692c843
Compare
yanzhudd
changed the title
az appconfig kv import: Support importing key-values from AKS ConfigMap
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
please fix the CI issues. |
|
Commenter does not have sufficient privileges for PR 31861 in repo Azure/azure-cli |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@yonzhan What's the difference between the instances? https://dev.azure.com/azclitools/public/_build/results?buildId=265753&view=results |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@yanzhudd The test was supposed to be fixed, could you please do a favor to try it again? |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Related command
az appconfig kv import --source aks
Description
Support importing key-values from AKS ConfigMap to provide a better user experience for migrating to use AppConfiguration as centralized configuration management in AKS.
Testing Guide
This 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.