-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
{Core} Add random_config_dir param in DummyCli #25689
Conversation
️✔️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
️✔️netappfiles
️✔️network
️✔️policyinsights
️✔️privatedns
️✔️profile
️✔️rdbms
️✔️redis
️✔️relay
️✔️resource
️✔️role
️✔️search
️✔️security
️✔️servicebus
️✔️serviceconnector
️✔️servicefabric
️✔️signalr
️✔️sql
️✔️sqlvm
️✔️storage
️✔️synapse
️✔️telemetry
️✔️util
️✔️vm
|
CI |
@@ -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'])]) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
@@ -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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
config_dir=os.path.join(GLOBAL_CONFIG_DIR, 'dummy_cli_config_dir', | ||
random_string()) if random_config_dir else GLOBAL_CONFIG_DIR, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. 😉
import shutil | ||
shutil.rmtree(self.cli_ctx.config.config_dir, ignore_errors=True) |
There was a problem hiding this comment.
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
def rmtree_with_retry(path): |
There was a problem hiding this comment.
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.
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) |
There was a problem hiding this comment.
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.
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
ARM64 machines are multi-core, and some errors occur because of concurrency, as all tests use same config dir.
I add a
random_config_dir
param inDummyCli
. If it is True, the config dir will be~/.azure/dummy_cli_config_dir/xxxxxxx
.If random config is enabled by default, the config file is created for each test, and the performance decrease a lot.
Azure.azure-cli Full Test
spends 24 min instead of 16min.I enable
random_config_dir
for the tests which useaz config
oraz configure
.Example usage:
History Notes
[Component Name 1] BREAKING CHANGE:
az command a
: Make some customer-facing breaking change[Component Name 2]
az command b
: Add some customer-facing featureThis 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.