Skip to content

{App Service} az webapp config container show: Fix accidental side-effect plus make sure this function handles both types of input data, lists and dict#24735

Open
antimirov wants to merge 2 commits intoAzure:devfrom
antimirov:feat/fix-accidental-input-value-side-effect
Open

{App Service} az webapp config container show: Fix accidental side-effect plus make sure this function handles both types of input data, lists and dict#24735
antimirov wants to merge 2 commits intoAzure:devfrom
antimirov:feat/fix-accidental-input-value-side-effect

Conversation

@antimirov
Copy link
Copy Markdown

@antimirov antimirov commented Nov 24, 2022 •
edited
Loading

The current implementation only supports dicts while its unit test explicitely passes a list of dicts as one of 6 cases. So it's actually broken in this regard.

Related command
N/A

Description
I've accidentally noticed a side-effect in the function _mask_creds_related_appsettings() - it's modifying the dictionary it's iterating over, so when it's returning a new dict, in fact the input dict is also being modified, which may lead to some failures later. While fixing it, I also noticed that it's supposed to work with both list or dicts and dicts, while not actually doing it.

In fact, this could be a bug, that this function should have only been used for one type of data, and it just accidentally does not break on lists while doing nothing basically, so difficult to spot the problem.

Testing Guide

# this is the command that I originally used
$ az webapp config container show

$ azdev test LinuxWebappScenarioTest --pytest-args "-n0 -s"

History Notes
N/A

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

@antimirov
fix accidental side-effect, plus make sure this function handles both…
704bcd0 
… types of imput data: lists and dict. The current one only supports dicts while unit test explicitely passing list of dicts as one of 6 cases
@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Nov 24, 2022
@ghost
Copy link
Copy Markdown

ghost commented Nov 24, 2022

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

@ghost ghost added the Auto-Assign Auto assign by bot label Nov 24, 2022
@ghost ghost assigned zhoxing-ms Nov 24, 2022
@ghost ghost added this to the Nov 2022 (2022-12-06) milestone Nov 24, 2022
@ghost ghost requested a review from yonzhan November 24, 2022 18:53
@ghost ghost added the Web Apps az webapp label Nov 24, 2022
@ghost ghost requested a review from wangzelin007 November 24, 2022 18:53
@antimirov antimirov changed the title azure-cli: fix accidental side-effect plus make sure this function handles both types of input data: lists and dict [azure-cli] fix accidental side-effect plus make sure this function handles both types of input data: lists and dict Nov 24, 2022
@antimirov antimirov changed the title [azure-cli] fix accidental side-effect plus make sure this function handles both types of input data: lists and dict [azure-cli] az webapp config container show: Fix accidental side-effect plus make sure this function handles both types of input data, lists and dict Nov 24, 2022
@antimirov antimirov changed the title [azure-cli] az webapp config container show: Fix accidental side-effect plus make sure this function handles both types of input data, lists and dict [azure-cli] az webapp config container show: Fix accidental side-effect plus make sure this function handles both types of input data, lists and dict Nov 24, 2022
@antimirov antimirov changed the title [azure-cli] az webapp config container show: Fix accidental side-effect plus make sure this function handles both types of input data, lists and dict {azure-cli} az webapp config container show: Fix accidental side-effect plus make sure this function handles both types of input data, lists and dict Nov 24, 2022
@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Nov 24, 2022

webapp

@zhoxing-ms zhoxing-ms changed the title {azure-cli} az webapp config container show: Fix accidental side-effect plus make sure this function handles both types of input data, lists and dict {App Service} az webapp config container show: Fix accidental side-effect plus make sure this function handles both types of input data, lists and dict Nov 30, 2022
@zhoxing-ms
Copy link
Copy Markdown
Contributor

zhoxing-ms commented Nov 30, 2022 •
edited
Loading

@StrawnSC Could you please help review the web app related PR?

@runefa
Copy link
Copy Markdown
Contributor

runefa commented Nov 30, 2022

Could you add a test for this case? Want to make sure everything is being correctly masked. Would also help to have a test for lists and dicts input.

@antimirov
Copy link
Copy Markdown
Author

antimirov commented Dec 12, 2022

@haroonf let me see what I can do

@zhoxing-ms
Copy link
Copy Markdown
Contributor

zhoxing-ms commented Jan 4, 2023

@antimirov Any update?

@zhoxing-ms
Copy link
Copy Markdown
Contributor

zhoxing-ms commented Jan 4, 2023

Please note that since we will launch the release of this sprint the day after tomorrow. If you cannot address all the comments tomorrow, we will have to postpone the release of this PR to the next sprint (02-07)

@zhoxing-ms
Copy link
Copy Markdown
Contributor

zhoxing-ms commented Feb 1, 2023

Could you add a test for this case? Want to make sure everything is being correctly masked. Would also help to have a test for lists and dicts input.

@antimirov Please note that since we will launch the release of this sprint the day after tomorrow. If you cannot address all the comments tomorrow, we will have to postpone the release of this PR to the next sprint (03-07)

@antimirov
Copy link
Copy Markdown
Author

antimirov commented Feb 1, 2023

@zhoxing-ms Sorry guys, I've been busy with some other tasks at my job. I could not finish any of the tests for this PR on time and I don't know when I'll be able to do it. No ETA at the moment. Maybe someone can help here? If you see the code change, you can judge its value. And whether reject this PR or not it's up to you. Apparently, when this code was produced, no tests were required and it was fine back then.

@zhoxing-ms
Copy link
Copy Markdown
Contributor

zhoxing-ms commented Feb 2, 2023

@antimirov Ok, I postpone the release of this PR to the next sprint. You can refer to document authoring_tests.md to write the tests

@haroonf Could you help antimirov add some relevant tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsntcy jsntcy Awaiting requested review from jsntcy

@zhoxing-ms zhoxing-ms Awaiting requested review from zhoxing-ms

@yanzhudd yanzhudd Awaiting requested review from yanzhudd yanzhudd is a code owner

@yonzhan yonzhan Awaiting requested review from yonzhan

@wangzelin007 wangzelin007 Awaiting requested review from wangzelin007

At least 1 approving review is required to merge this pull request.

Assignees

@zhoxing-ms zhoxing-ms

Labels

act-observability-squad Auto-Assign Auto assign by bot customer-reported Issues that are reported by GitHub users external to the Azure organization. Web Apps az webapp

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

5 participants

@antimirov @yonzhan @zhoxing-ms @runefa @a0x1ab