Skip to content

{Core} Add random_config_dir param in DummyCli #25689

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

Merged
bebound merged 13 commits into from
Apr 13, 2023
Merged

{Core} Add random_config_dir param in DummyCli #25689

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
43813d2
Use random config dir in DummyCli
bebound Mar 6, 2023
109201c
Use subdir
bebound Mar 6, 2023
b94d580
Fix test
bebound Mar 8, 2023
1b87eea
Remove warning
bebound Mar 8, 2023
869aacb
Fix test
bebound Mar 8, 2023
99aeaf0
Get real config dir from cli_ctx.config
bebound Mar 8, 2023
c29ff94
Fix path
bebound Mar 8, 2023
6a6ca82
Add random_config_dir param
bebound Mar 8, 2023
60afa32
Minor change
bebound Mar 9, 2023
7a8d2bf
Use random folder
bebound Mar 24, 2023
d777ac0
Use rmtree
bebound Mar 24, 2023
7f9e0d4
Minor fix
bebound Mar 27, 2023
0ec2406
Minor fix
bebound Mar 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/azure-cli-core/azure/cli/core/_session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,11 @@ def load(self, filename, max_age=0):
if isinstance(load_exception, json.JSONDecodeError):
log_level = logging.WARNING

get_logger(__name__).log(log_level,
"Failed to load or parse file %s. It will be overridden by default settings.",
self.filename)
# DummyCLI uses random config folder, it will always use default settings. This avoids unnecessary warning.
if 'PYTEST_CURRENT_TEST' not in os.environ:
get_logger(__name__).log(log_level,
"Failed to load or parse file %s. It will be overridden by default settings.",
self.filename)
self.save()

def save(self):
Expand Down
6 changes: 5 additions & 1 deletion src/azure-cli-core/azure/cli/core/mock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,19 @@ def __init__(self, commands_loader_cls=None, **kwargs):
from azure.cli.core.azlogging import AzCliLogging
from azure.cli.core.cloud import get_active_cloud
from azure.cli.core.parser import AzCliCommandParser
from azure.cli.core.util import random_string
from azure.cli.core._config import GLOBAL_CONFIG_DIR, ENV_VAR_PREFIX
from azure.cli.core._help import AzCliHelp
from azure.cli.core._output import AzOutputProducer

from knack.completion import ARGCOMPLETE_ENV_NAME

random_config_dir = kwargs.get('random_config_dir', False)
Copy link
Member

@jiasli jiasli Mar 17, 2023

Choose a reason for hiding this comment

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

Consider making it an explicit kwarg.


super(DummyCli, self).__init__(
cli_name='az',
config_dir=GLOBAL_CONFIG_DIR,
config_dir=os.path.join(GLOBAL_CONFIG_DIR, 'dummy_cli_config_dir',
random_string()) if random_config_dir else GLOBAL_CONFIG_DIR,
Comment on lines +28 to +29
Copy link
Member

@jiasli jiasli Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

As an alternative, we may also consider using tempfile module: https://docs.python.org/3/library/tempfile.html

Copy link
Member

@jiasli jiasli Mar 27, 2023

Choose a reason for hiding this comment

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

Well, sorry I didn't make my point clear. I am simply providing an alternative solution. You solution is actually better since your location is easy to find. 😉

config_env_var_prefix=ENV_VAR_PREFIX,
commands_loader_cls=commands_loader_cls or MainCommandsLoader,
parser_cls=AzCliCommandParser,
Expand Down
12 changes: 9 additions & 3 deletions src/azure-cli-testsdk/azure/cli/testsdk/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,10 @@ def is_empty(self): # pylint: disable=no-self-use

class ScenarioTest(ReplayableTest, CheckerMixin, unittest.TestCase):
def __init__(self, method_name, config_file=None, recording_name=None,
recording_processors=None, replay_processors=None, recording_patches=None, replay_patches=None):
self.cli_ctx = get_dummy_cli()
recording_processors=None, replay_processors=None, recording_patches=None, replay_patches=None,
random_config_dir=False):
self.cli_ctx = get_dummy_cli(random_config_dir=random_config_dir)
self.random_config_dir = random_config_dir
self.name_replacer = GeneralNameReplacer()
self.kwargs = {}
self.test_guid_count = 0
Expand Down Expand Up @@ -137,6 +139,9 @@ def _merge_lists(base, patches):
def tearDown(self):
for processor in self._processors_to_reset:
processor.reset()
if self.random_config_dir:
import shutil
shutil.rmtree(self.cli_ctx.config.config_dir, ignore_errors=True)
Copy link
Member

@jiasli jiasli Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Well... yes. rmtree can fail intermittently on Windows. Consider using

azure-cli/src/azure-cli-core/azure/cli/core/util.py

Line 1317 in 5757778

def rmtree_with_retry(path):

Copy link
Contributor Author

@bebound bebound Mar 17, 2023

Choose a reason for hiding this comment

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

Oh, yes. I forget that Windows bug.

super(ScenarioTest, self).tearDown()

def create_random_name(self, prefix, length):
Expand Down Expand Up @@ -181,7 +186,8 @@ class LocalContextScenarioTest(ScenarioTest):
def __init__(self, method_name, config_file=None, recording_name=None, recording_processors=None,
replay_processors=None, recording_patches=None, replay_patches=None, working_dir=None):
super(LocalContextScenarioTest, self).__init__(method_name, config_file, recording_name, recording_processors,
replay_processors, recording_patches, replay_patches)
replay_processors, recording_patches, replay_patches,
random_config_dir=True)
Copy link
Contributor Author

@bebound bebound Mar 9, 2023

Choose a reason for hiding this comment

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

ConfigureGlobalDefaultsTest runs config param-persist off in tearDown. I think use random_config_dir is necessary

Copy link
Member

@jiasli jiasli Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Local Context/Param Persist has been deprecated. Consider deleting related tests instead: #24726

if self.in_recording:
self.recording_patches.append(patch_get_current_system_username)
else:
Expand Down
9 changes: 6 additions & 3 deletions src/azure-cli/azure/cli/command_modules/config/tests/latest/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@

class ConfigTest(ScenarioTest):

def __init__(self, *arg, **kwargs):
super().__init__(*arg, random_config_dir=True, **kwargs)

def test_config(self):

# [test_section1]
Expand All @@ -28,8 +31,9 @@ def test_config(self):
os.chdir(tempdir)
print("Using temp dir: {}".format(tempdir))

global_test_args = {"source": os.path.expanduser(os.path.join('~', '.azure', 'config')), "flag": ""}
local_test_args = {"source": os.path.join(tempdir, '.azure', 'config'), "flag": " --local"}
global_test_args = {"source": os.path.join(self.cli_ctx.config.config_dir, 'config'), "flag": ""}
local_test_args = {"source": os.path.join(tempdir, os.path.basename(self.cli_ctx.config.config_dir), 'config'),
"flag": " --local"}

for args in (global_test_args, local_test_args):
print("Testing for {}".format(args))
Expand Down Expand Up @@ -75,7 +79,6 @@ def test_config(self):
self.cmd('config unset test_section1.test_option1' + args['flag'])
# Test unsetting multiple options
self.cmd('config unset test_section2.test_option21 test_section2.test_option22' + args['flag'])

os.chdir(original_path)


Expand Down
3 changes: 3 additions & 0 deletions src/azure-cli/azure/cli/command_modules/configure/tests/latest/test_configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ def test_configure_output_options(self):

class ConfigureGlobalDefaultsTest(ScenarioTest):

def __init__(self, *arg, **kwargs):
super().__init__(*arg, random_config_dir=True, **kwargs)

def setUp(self):
self.local_dir = tempfile.mkdtemp()

Expand Down
3 changes: 3 additions & 0 deletions src/azure-cli/azure/cli/command_modules/resource/tests/latest/test_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,9 @@ def test_resource_create_and_show(self, resource_group, resource_group_location)

class TagScenarioTest(ScenarioTest):

def __init__(self, *arg, **kwargs):
super().__init__(*arg, random_config_dir=True, **kwargs)

def test_tag_scenario(self):

self.kwargs.update({
Expand Down
5 changes: 4 additions & 1 deletion src/azure-cli/azure/cli/command_modules/role/tests/latest/test_role.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,9 @@ def test_role_definition_scenario(self):

class RoleAssignmentScenarioTest(RoleScenarioTestBase):

def __init__(self, *arg, **kwargs):
super().__init__(*arg, random_config_dir=True, **kwargs)
Copy link
Member

@jiasli jiasli Aug 14, 2023

Choose a reason for hiding this comment

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

random_config_dir=True make tests under RoleAssignmentScenarioTest fail during live run, because the login context is lost.

Copy link
Contributor Author

@bebound bebound Aug 14, 2023

Choose a reason for hiding this comment

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

Yes, we need to merge #26475 first.


@ResourceGroupPreparer(name_prefix='cli_role_assign')
@AllowLargeResponse()
def test_role_assignment_scenario(self, resource_group):
Expand Down Expand Up @@ -568,7 +571,7 @@ def test_role_assignment_handle_conflicted_assignments(self, resource_group):

self.assertGreaterEqual(len(local_defaults_config), 1)
actual = set([(x['name'], x['source'], x['value']) for x in local_defaults_config if x['name'] == 'group'])
expected = set([('group', os.path.join(temp_dir, '.azure', 'config'), self.kwargs['rg'])])
expected = set([('group', os.path.join(temp_dir, os.path.basename(self.cli_ctx.config.config_dir), 'config'), self.kwargs['rg'])])
Copy link
Contributor Author

@bebound bebound Mar 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The config path is temp_dir+basename(config_dir)+'config' when --scope local

Copy link
Member

@jiasli jiasli Mar 17, 2023

Choose a reason for hiding this comment

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

Let's provide some concrete example.

Copy link
Contributor Author

@bebound bebound Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If work dir is C:\Users\hanglei\AppData\Local\Temp\tmplxf_89ez, local config file is C:\Users\hanglei\AppData\Local\Temp\tmplxf_89ez\0azXbKR9OdJuZPFS\config

0azXbKR9OdJuZPFS is the base name of config dir: ~/.azure/dummy_cli_config_dir/0azXbKR9OdJuZPFS/

Copy link
Member

@jiasli jiasli Mar 29, 2023

Choose a reason for hiding this comment

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

The local config folder was constructed by knack.config.CLIConfig:

https://github.com/microsoft/knack/blob/e496c9590792572e680cb3ec959db175d9ba85dd/knack/config.py#L63

config_dir_name = os.path.basename(self.config_dir)
current_config_dir = os.path.join(current_dir, config_dir_name)

current_config_dir is constructed by combining current_dir with the random dir name, making the path a little bit weird.

self.assertEqual(actual, expected)

# test role assignments on a resource group
Expand Down