Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Config] Make config items case-insensitive #220

Merged
merged 7 commits into from
Dec 9, 2020
Merged

[Config] Make config items case-insensitive #220

merged 7 commits into from
Dec 9, 2020

Conversation

evelyn-ys
Copy link
Contributor

@evelyn-ys evelyn-ys commented Sep 28, 2020 •
edited by jiasli
Loading

Description

Fix Azure/azure-cli#14397: az configure --list-defaults should not be case sensitive for environment variables

Testing Guide

preparation(need Azure CLI in advance)

az configure --defaults location=eastus
export AZURE_DEFAULTS_LOCATION=eastasia

test

az configure --list-defaults

results before

[
  {
    "name": "LOCATION",
    "source": "AZURE_DEFAULTS_LOCATION",
    "value": "eastasia"
  },
  {
    "name": "location",
    "source": "C:\\Users\\yishiwang\\.azure\\config",
    "value": "eastus"
  }
]

results after

[
  {
    "name": "LOCATION",
    "source": "AZURE_DEFAULTS_LOCATION",
    "value": "eastasia"
  }
]

@evelyn-ys evelyn-ys changed the title {Config}config items should not be case sensitive {Config} config items should not be case sensitive Sep 28, 2020
jiasli
jiasli reviewed Sep 28, 2020
View reviewed changes
tests/test_config.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
jiasli
jiasli reviewed Sep 28, 2020
View reviewed changes
knack/config.py Outdated
for config in self._config_file_chain if self.use_local_config else self._config_file_chain[-1:]:
try:
entries = config.items(section)
for name, value in entries:
if name not in result:
if name.lower() not in result:
Copy link
Member

Choose a reason for hiding this comment

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

We should force lower case for all configs in files, otherwise we need to change get and set as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall we add force rule in set_value?

knack/config.py Outdated Show resolved Hide resolved
jiasli
jiasli reviewed Oct 27, 2020
View reviewed changes
knack/config.py Outdated
@@ -109,9 +109,9 @@ def sections(self):

def items(self, section):
import re
pattern = self.env_var_name(section, '.+')
pattern = self.env_var_name(section, '[0-9A-Z]+')
Copy link
Member

Choose a reason for hiding this comment

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

The option name can consist of multiple parts, like AZURE_BATCHAI_STORAGE_ACCOUNT.

@evelyn-ys evelyn-ys requested a review from jiasli October 27, 2020 08:59
jiasli
jiasli reviewed Dec 2, 2020
View reviewed changes
knack/config.py Outdated
candidates = [(k.split('_')[-1], os.environ[k], k) for k in os.environ if re.match(pattern, k)]
result = {c[0]: c for c in candidates}
pattern = self.env_var_name(section, '[0-9A-Z][0-9A-Z_]*')
candidates = [(k.replace(self.env_var_name(section, ''), ''), os.environ[k], k) for k in os.environ if re.match(pattern, k)]
Copy link
Member

Choose a reason for hiding this comment

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

replace can cause unwanted effect if self.env_var_name(section, '') somehow appears in the middle of the env var. removeprefix is a better choice but it is only available since Python 3.9.

Better to use grouping of regex.

jiasli
jiasli reviewed Dec 2, 2020
View reviewed changes
tests/test_config.py Outdated
Comment on lines 174 to 177
envOption = '{}_{}_{}'.format(CLIConfig._DEFAULT_CONFIG_ENV_VAR_PREFIX, section, 'TEST_OPTION')
envVaule = 'envValue'
configOption = 'test_option'
configValue = 'value'
Copy link
Member

Choose a reason for hiding this comment

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

The python convention is to use snake case.

Copy link

Choose a reason for hiding this comment

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

The python convention is to use snake case.

Agree

jiasli
jiasli reviewed Dec 2, 2020
View reviewed changes
knack/config.py Outdated
pattern = self.env_var_name(section, '.+')
candidates = [(k.split('_')[-1], os.environ[k], k) for k in os.environ if re.match(pattern, k)]
result = {c[0]: c for c in candidates}
pattern = self.env_var_name(section, '[0-9A-Z][0-9A-Z_]*')
Copy link
Member

@jiasli jiasli Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This will match CLI_MYSECTION_T part in CLI_MYSECTION_Test_Option, defeating our purpose of ignoring invalid env options like CLI_MYSECTION_Test_Option. We should use fullmatch instead.

@jiasli jiasli changed the title {Config} config items should not be case sensitive [Config] Make config items case-insensitive Dec 2, 2020
@evelyn-ys
Copy link
Contributor Author

looks good now

@jiasli jiasli merged commit 676477e into microsoft:master Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qwordy qwordy qwordy approved these changes

@jiasli jiasli jiasli approved these changes

@arrownj arrownj arrownj approved these changes

Assignees

@evelyn-ys evelyn-ys

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

az configure --list-defaults should not be case sensitive for environment variables
4 participants
@evelyn-ys @arrownj @jiasli @qwordy