[App Config] `az appconfig`: Add warning log info to output when the `--yes` flag is set #25560

albertofori · 2023-02-24T06:37:03Z

Related command

az appconfig import
az appconfig export

Description

Related to Azure/AppConfiguration issue #712. This fix separates kv diffing from print previews and enables logging of specific information to output even when the --yes flag is set.

Testing Guide

History Notes

[App Config] az appconfig import/export: Add warning log info to output even when --yes flag is set

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.

yonzhan · 2023-02-24T09:38:48Z

App Config

albertofori · 2023-02-24T17:26:12Z

Hi @avanigupta, Could you kindly take a look at this? Thanks!

src/azure-cli/azure/cli/command_modules/appconfig/_kv_helpers.py

src/azure-cli/azure/cli/command_modules/appconfig/_constants.py

yanzhudd · 2023-03-02T04:42:28Z

src/azure-cli/azure/cli/command_modules/appconfig/_kv_helpers.py

@@ -637,30 +693,26 @@ def __print_features_preview(old_json, new_json, strict=False):
    res = differ.diff(old_json, new_json)
    keys = str(res.keys())
    if res == {} or (('update' not in keys) and ('insert' not in keys) and (not strict or ('delete' not in keys))):
-        logger.warning('\nTarget configuration already contains all feature flags in source. No changes will be made.')
-        return False
+        return ff_diff


Could you please add a comment for this if statement?

yanzhudd · 2023-03-02T04:47:09Z

src/azure-cli/azure/cli/command_modules/appconfig/_kv_helpers.py

@@ -689,28 +739,23 @@ def __print_preview(old_json, new_json, strict=False):
    res = differ.diff(old_json, new_json)
    keys = str(res.keys())
    if res == {} or (('update' not in keys) and ('insert' not in keys) and (not strict or ('delete' not in keys))):
-        logger.warning('\nTarget configuration already contains all key-values in source. No changes will be made.')
-        return False
+        return kv_diff


Could you please add a comment for this if statement?

zhoxing-ms · 2023-03-02T09:24:31Z

I'd like to confirm with you why it is marked as a breaking change? Will printing the log cause a breaking change to the user's script?

zhoxing-ms · 2023-03-02T09:27:53Z

Please note that we will launch the release for this sprint tomorrow morning. Since there are still some comments above, so we postpone the release time of this PR to the next sprint (04-04), could you accept this release time?

albertofori · 2023-03-02T09:28:35Z

@zhoxing-ms Yes please. For users who have scripts that should terminate on warnings, this could potentially cause the script to break. Just a precaution on this.

albertofori · 2023-03-02T09:30:27Z

Since the only outstanding ones are for adding comments, could we make the release tomorrow if I address those comments and push the changes now?

azure-client-tools-bot-prd · 2023-03-02T09:48:18Z

️✔️acr

️✔️acs

️✔️advisor

️✔️ams

️✔️apim

️✔️appconfig

️✔️appservice

️✔️aro

️✔️backup

️✔️batch

️✔️batchai

️✔️billing

️✔️botservice

️✔️cdn

️✔️cloud

️✔️cognitiveservices

️✔️config

️✔️configure

️✔️consumption

️✔️container

️✔️core

️✔️cosmosdb

️✔️databoxedge

️✔️dla

️✔️dls

️✔️dms

️✔️eventgrid

️✔️eventhubs

️✔️feedback

️✔️find

️✔️hdinsight

️✔️identity

️✔️iot

️✔️keyvault

️✔️kusto

️✔️lab

️✔️managedservices

️✔️maps

️✔️marketplaceordering

️✔️monitor

️✔️natgateway

️✔️netappfiles

️✔️network

️✔️policyinsights

️✔️privatedns

️✔️profile

️✔️rdbms

️✔️redis

️✔️relay

️✔️resource

️✔️role

️✔️search

️✔️security

️✔️servicebus

️✔️serviceconnector

️✔️servicefabric

️✔️signalr

️✔️sql

️✔️sqlvm

️✔️storage

️✔️synapse

️✔️telemetry

️✔️util

️✔️vm

albertofori · 2023-03-03T18:18:20Z

Hi @zhoxing-ms. I just wanted to confirm if this will make the release window as the comments have been addressed.

zhoxing-ms · 2023-03-06T03:33:41Z

OK, so I'd like to confirm with you that the breaking change contained in this PR only refers to the warning log at present?
If so, in fact, the warning log is not strictly a breaking change for CLI, because it will not directly affect the input, output and command's behavior

Anino1996 · 2023-03-06T03:37:13Z

Yes please. That’s the only change here. The input and returned output behaviour does not change for users.

zhoxing-ms · 2023-03-06T03:47:12Z

I'm so sorry for my late reply. I didn't see your comments in time because I was super busy last Friday.
Because this PR was created late (we usually recommend to submit PR one week before the code freeze date, which can leave us enough time to review and improve PR) and may contain breaking changes, we did not release it in this sprint for the sake of the impact on users and breaking change process.
Since the breaking change contained in this PR only refers to the warning log, we don't need to wait for the next breaking change release. We will release it at the next sprint (04-04) as soon as possible, may I ask could you accept this release time?

albertofori · 2023-03-06T04:08:22Z

@zhoxing-ms No worries. The 04-04 release time should be fine. Thanks!

zhoxing-ms · 2023-03-06T06:09:24Z

Thanks for your understanding~

zhoxing-ms · 2023-03-06T07:13:36Z

@albertofori Since we usually do not add the breaking change tag for logger.warning, So I modified your PR title and history notes

…`--yes` flag is set (Azure#25560)

albertofori added 2 commits February 23, 2023 20:41

Separating diff and preview functions.

b33fdaf

Adding test recording files.

e246771

ghost requested review from wangzelin007, zhoxing-ms and yonzhan February 24, 2023 06:37

ghost assigned zhoxing-ms Feb 24, 2023

ghost added this to the Feb 2023 (2023-03-07) milestone Feb 24, 2023

ghost requested a review from jsntcy February 24, 2023 06:37

ghost added App Configuration Auto-Assign Auto assign by bot labels Feb 24, 2023

Resolving merge conflicts.

0c3480f

Adding test recording file.

3d59a36

albertofori marked this pull request as ready for review February 24, 2023 18:17

albertofori requested a review from yanzhudd as a code owner February 24, 2023 18:17

avanigupta reviewed Feb 24, 2023

View reviewed changes

src/azure-cli/azure/cli/command_modules/appconfig/_kv_helpers.py Outdated Show resolved Hide resolved

avanigupta reviewed Feb 24, 2023

View reviewed changes

src/azure-cli/azure/cli/command_modules/appconfig/_constants.py Outdated Show resolved Hide resolved

albertofori added 4 commits February 24, 2023 12:02

Removing duplicate logging.

555e4ca

Renaming json diff constants.

a0d5c77

Fixing linting issues.

b834a96

Fixing omitted KVSet preview header.

20a6365

avanigupta previously approved these changes Feb 24, 2023

View reviewed changes

zhoxing-ms assigned yanzhudd Feb 27, 2023

Fixing formatting issues.

31bbed9

albertofori dismissed avanigupta’s stale review via 31bbed9 February 28, 2023 09:29

yanzhudd reviewed Mar 2, 2023

View reviewed changes

zhoxing-ms modified the milestones: Feb 2023 (2023-03-07), March 2023 (2023-04-04) Mar 2, 2023

Adding comments.

bd49d26

zhoxing-ms changed the title ~~[App Config] BREAKING CHANGE: az appconfig: Adding logging to output when the "yes" flag is set~~ [App Config] az appconfig: Add logging to output when the --yes flag is set Mar 6, 2023

zhoxing-ms changed the title ~~[App Config] az appconfig: Add logging to output when the --yes flag is set~~ [App Config] az appconfig: Add warning log info to output when the --yes flag is set Mar 6, 2023

zhoxing-ms approved these changes Mar 6, 2023

View reviewed changes

yanzhudd approved these changes Mar 6, 2023

View reviewed changes

zhoxing-ms merged commit 259eb96 into Azure:dev Mar 6, 2023

brental mentioned this pull request Mar 15, 2023

No logs with App Configuration export when 'yes' flag is setted Azure/AppConfiguration#712

Closed

avgale pushed a commit to avgale/azure-cli that referenced this pull request Aug 24, 2023

[App Config] az appconfig: Add warning log info to output when the …

85cde51

…`--yes` flag is set (Azure#25560)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Config] `az appconfig`: Add warning log info to output when the `--yes` flag is set #25560

[App Config] `az appconfig`: Add warning log info to output when the `--yes` flag is set #25560

albertofori commented Feb 24, 2023 •

edited by zhoxing-ms

Loading

yonzhan commented Feb 24, 2023

albertofori commented Feb 24, 2023

yanzhudd Mar 2, 2023

yanzhudd Mar 2, 2023

zhoxing-ms commented Mar 2, 2023

zhoxing-ms commented Mar 2, 2023

albertofori commented Mar 2, 2023

albertofori commented Mar 2, 2023

azure-client-tools-bot-prd bot commented Mar 2, 2023 •

edited

Loading

albertofori commented Mar 3, 2023

zhoxing-ms commented Mar 6, 2023

Anino1996 commented Mar 6, 2023

zhoxing-ms commented Mar 6, 2023 •

edited

Loading

albertofori commented Mar 6, 2023

zhoxing-ms commented Mar 6, 2023

zhoxing-ms commented Mar 6, 2023 •

edited

Loading

[App Config] az appconfig: Add warning log info to output when the --yes flag is set #25560

[App Config] az appconfig: Add warning log info to output when the --yes flag is set #25560

Conversation

albertofori commented Feb 24, 2023 • edited by zhoxing-ms Loading

yonzhan commented Feb 24, 2023

albertofori commented Feb 24, 2023

yanzhudd Mar 2, 2023

Choose a reason for hiding this comment

yanzhudd Mar 2, 2023

Choose a reason for hiding this comment

zhoxing-ms commented Mar 2, 2023

zhoxing-ms commented Mar 2, 2023

albertofori commented Mar 2, 2023

albertofori commented Mar 2, 2023

azure-client-tools-bot-prd bot commented Mar 2, 2023 • edited Loading

albertofori commented Mar 3, 2023

zhoxing-ms commented Mar 6, 2023

Anino1996 commented Mar 6, 2023

zhoxing-ms commented Mar 6, 2023 • edited Loading

albertofori commented Mar 6, 2023

zhoxing-ms commented Mar 6, 2023

zhoxing-ms commented Mar 6, 2023 • edited Loading

[App Config] `az appconfig`: Add warning log info to output when the `--yes` flag is set #25560

[App Config] `az appconfig`: Add warning log info to output when the `--yes` flag is set #25560

albertofori commented Feb 24, 2023 •

edited by zhoxing-ms

Loading

azure-client-tools-bot-prd bot commented Mar 2, 2023 •

edited

Loading

zhoxing-ms commented Mar 6, 2023 •

edited

Loading

zhoxing-ms commented Mar 6, 2023 •

edited

Loading