Skip to content

[App Config] az appconfig kv import/export/restore: Add dry-run feature#30842

Merged
zhoxing-ms merged 8 commits intoAzure:devfrom
ChristineWanjau:cwanjau/addDryRunFeature
Jun 23, 2025
Merged

[App Config] az appconfig kv import/export/restore: Add dry-run feature#30842
zhoxing-ms merged 8 commits intoAzure:devfrom
ChristineWanjau:cwanjau/addDryRunFeature

Conversation

@ChristineWanjau
Copy link
Copy Markdown
Contributor

@ChristineWanjau ChristineWanjau commented Feb 18, 2025

Related command

az appconfig kv import
az appconfig kv export
az appconfig kv restore

Description

This PR adds a new argument --dry-run to import/export/restore commands. This just previews the changes on the console without making actual changes to the appconfig store.
Feature request - Azure/AppConfiguration#1014

Testing Guide

History Notes

[App Config] az appconfig kv import/export/restore: Add new parameter --dry-run to support dry-run feature


This checklist is used to make sure that common guidelines for a pull request are followed.

@azure-client-tools-bot-prd
Copy link
Copy Markdown

Hi @ChristineWanjau,
Since the current milestone time is less than 7 days, this pr will be reviewed in the next milestone.

@azure-client-tools-bot-prd
Copy link
Copy Markdown

azure-client-tools-bot-prd Bot commented Feb 18, 2025

️✔️AzureCLI-FullTest
️✔️acr
️✔️latest
️✔️3.12
️✔️3.9
️✔️acs
️✔️latest
️✔️3.12
️✔️3.9
️✔️advisor
️✔️latest
️✔️3.12
️✔️3.9
️✔️ams
️✔️latest
️✔️3.12
️✔️3.9
️✔️apim
️✔️latest
️✔️3.12
️✔️3.9
️✔️appconfig
️✔️latest
️✔️3.12
️✔️3.9
️✔️appservice
️✔️latest
️✔️3.12
️✔️3.9
️✔️aro
️✔️latest
️✔️3.12
️✔️3.9
️✔️backup
️✔️latest
️✔️3.12
️✔️3.9
️✔️batch
️✔️latest
️✔️3.12
️✔️3.9
️✔️batchai
️✔️latest
️✔️3.12
️✔️3.9
️✔️billing
️✔️latest
️✔️3.12
️✔️3.9
️✔️botservice
️✔️latest
️✔️3.12
️✔️3.9
️✔️cdn
️✔️latest
️✔️3.12
️✔️3.9
️✔️cloud
️✔️latest
️✔️3.12
️✔️3.9
️✔️cognitiveservices
️✔️latest
️✔️3.12
️✔️3.9
️✔️compute_recommender
️✔️latest
️✔️3.12
️✔️3.9
️✔️computefleet
️✔️latest
️✔️3.12
️✔️3.9
️✔️config
️✔️latest
️✔️3.12
️✔️3.9
️✔️configure
️✔️latest
️✔️3.12
️✔️3.9
️✔️consumption
️✔️latest
️✔️3.12
️✔️3.9
️✔️container
️✔️latest
️✔️3.12
️✔️3.9
️✔️containerapp
️✔️latest
️✔️3.12
️✔️3.9
️✔️core
️✔️latest
️✔️3.12
️✔️3.9
️✔️cosmosdb
️✔️latest
️✔️3.12
️✔️3.9
️✔️databoxedge
️✔️latest
️✔️3.12
️✔️3.9
️✔️dls
️✔️latest
️✔️3.12
️✔️3.9
️✔️dms
️✔️latest
️✔️3.12
️✔️3.9
️✔️eventgrid
️✔️latest
️✔️3.12
️✔️3.9
️✔️eventhubs
️✔️latest
️✔️3.12
️✔️3.9
️✔️feedback
️✔️latest
️✔️3.12
️✔️3.9
️✔️find
️✔️latest
️✔️3.12
️✔️3.9
️✔️hdinsight
️✔️latest
️✔️3.12
️✔️3.9
️✔️identity
️✔️latest
️✔️3.12
️✔️3.9
️✔️iot
️✔️latest
️✔️3.12
️✔️3.9
️✔️keyvault
️✔️latest
️✔️3.12
️✔️3.9
️✔️lab
️✔️latest
️✔️3.12
️✔️3.9
️✔️managedservices
️✔️latest
️✔️3.12
️✔️3.9
️✔️maps
️✔️latest
️✔️3.12
️✔️3.9
️✔️marketplaceordering
️✔️latest
️✔️3.12
️✔️3.9
️✔️monitor
️✔️latest
️✔️3.12
️✔️3.9
️✔️mysql
️✔️latest
️✔️3.12
️✔️3.9
️✔️netappfiles
️✔️latest
️✔️3.12
️✔️3.9
️✔️network
️✔️latest
️✔️3.12
️✔️3.9
️✔️policyinsights
️✔️latest
️✔️3.12
️✔️3.9
️✔️privatedns
️✔️latest
️✔️3.12
️✔️3.9
️✔️profile
️✔️latest
️✔️3.12
️✔️3.9
️✔️rdbms
️✔️latest
️✔️3.12
️✔️3.9
️✔️redis
️✔️latest
️✔️3.12
️✔️3.9
️✔️relay
️✔️latest
️✔️3.12
️✔️3.9
️✔️resource
️✔️latest
️✔️3.12
️✔️3.9
️✔️role
️✔️latest
️✔️3.12
️✔️3.9
️✔️search
️✔️latest
️✔️3.12
️✔️3.9
️✔️security
️✔️latest
️✔️3.12
️✔️3.9
️✔️servicebus
️✔️latest
️✔️3.12
️✔️3.9
️✔️serviceconnector
️✔️latest
️✔️3.12
️✔️3.9
️✔️servicefabric
️✔️latest
️✔️3.12
️✔️3.9
️✔️signalr
️✔️latest
️✔️3.12
️✔️3.9
️✔️sql
️✔️latest
️✔️3.12
️✔️3.9
️✔️sqlvm
️✔️latest
️✔️3.12
️✔️3.9
️✔️storage
️✔️latest
️✔️3.12
️✔️3.9
️✔️synapse
️✔️latest
️✔️3.12
️✔️3.9
️✔️telemetry
️✔️latest
️✔️3.12
️✔️3.9
️✔️util
️✔️latest
️✔️3.12
️✔️3.9
️✔️vm
️✔️latest
️✔️3.12
️✔️3.9

@azure-client-tools-bot-prd
Copy link
Copy Markdown

azure-client-tools-bot-prd Bot commented Feb 18, 2025

⚠️AzureCLI-BreakingChangeTest
⚠️appconfig
rule cmd_name rule_message suggest_message
⚠️ 1006 - ParaAdd appconfig kv export cmd appconfig kv export added parameter dry_run
⚠️ 1006 - ParaAdd appconfig kv import cmd appconfig kv import added parameter dry_run
⚠️ 1006 - ParaAdd appconfig kv restore cmd appconfig kv restore added parameter dry_run

@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Feb 18, 2025

Thank you for your contribution! We will review the pull request and get back to you soon.

@github-actions
Copy link
Copy Markdown

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).
After that please run the following commands to enable git hooks:

pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>

@ChristineWanjau ChristineWanjau marked this pull request as ready for review February 18, 2025 13:06
@ChristineWanjau ChristineWanjau changed the title [App config]az appconfig kv import/export/restore: Add dry-run feature [App config]az appconfig kv import/export/restore: Add dry-run feature Feb 18, 2025
@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Feb 18, 2025

Please fix CI issues

@zhoxing-ms
Copy link
Copy Markdown
Contributor

Please resolve these CI issues and add some test cases

@zhoxing-ms
Copy link
Copy Markdown
Contributor

Please note that Azure CLI will have a code freeze on 03/25/2025 07:00 UTC for the upcoming release. If you want to catch this release train, please address the commands ASAP, otherwise it has to be postponed to next sprint (05-06)

@yanzhudd
Copy link
Copy Markdown
Contributor

please note that Azure CLI will have a code freeze on 04/21/2025 07:00 UTC for the upcoming release. Please address the comments ASAP, otherwise it has to be postponed to next sprint (05-19).

@yanzhudd yanzhudd changed the title [App config]az appconfig kv import/export/restore: Add dry-run feature [App Config] az appconfig kv import/export/restore: Add dry-run feature Apr 21, 2025
@zhoxing-ms
Copy link
Copy Markdown
Contributor

Any update?

@zhoxing-ms zhoxing-ms added this to the Backlog milestone May 12, 2025
return

# If yes is provided, it should take precedence over dry-run
if dry_run and not yes:
Copy link
Copy Markdown
Contributor

@albertofori albertofori Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dry-run is supposed to be a safety net. Is there a reason --yes would take precedence over it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not allow dry-run and --yes to be specified together.

Copy link
Copy Markdown
Contributor

@albertofori albertofori Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, dry-run should not lead to a point of user confirmation so --yes should not be applicable

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Added validation to not allow dry-run and yes to specified together


need_change = __print_restore_preview(restore_diff, yes=yes)

if dry_run and not yes:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can validate that dry-run and yes are not specified together and return if only dry-run is True

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

c.argument('skip_features', help="Import only key values and exclude all feature flags. By default, all feature flags will be imported from file or appconfig. Not applicable for appservice.", arg_type=get_three_state_flag())
c.argument('content_type', help='Content type of all imported items.')
c.argument('import_mode', arg_type=get_enum_type([ImportMode.ALL, ImportMode.IGNORE_MATCH]), help='If import mode is "ignore-match", only source key-values that do not already exist or whose value, content-type or tags are different from that of an existing key-value with the same key and label, will be written. Import mode "all" writes all key-values to the destination regardless of whether they exist or not.')
c.argument('dry_run', validator=validate_dry_run, help="Perform a dry-run to validate the import operation without making changes to the App Configurations store. Any updates that would normally occur will be printed to the console for review.")
Copy link
Copy Markdown
Member

@avanigupta avanigupta Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
c.argument('dry_run', validator=validate_dry_run, help="Perform a dry-run to validate the import operation without making changes to the App Configurations store. Any updates that would normally occur will be printed to the console for review.")
c.argument('dry_run', validator=validate_dry_run, help="Preview the result of import operation without making any changes to the App Configuration store.")

if not need_kv_change and not need_feature_change:
return

if dry_run:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dry-run is not honored when profile is kvset

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for import/export both

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added dry-run for kvset

return

if not yes:
if need_change is False:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt we check for need_change before checking for yes?
The debug message looks unnecessary. I think we can follow the same pattern as import/export here:

    if not need_change:
        return

    if dry_run:
        return

    # if customer needs preview & confirmation
    if not yes:
        user_confirmation("Do you want to continue? \n")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we can combine the checks for need_change and dry_run:

    if not need_change or dry_run:
        return

    # if customer needs preview & confirmation
    if not yes:
        user_confirmation("Do you want to continue? \n")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. Updated

Comment on lines 223 to +227
if not need_kv_change and not need_feature_change:
return

if dry_run:
return
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not need_kv_change and not need_feature_change:
return
if dry_run:
return
if (not need_kv_change and not need_feature_change) or (dry_run):
return

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@zhoxing-ms zhoxing-ms merged commit e1f5641 into Azure:dev Jun 23, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants