-
Notifications
You must be signed in to change notification settings - Fork 3.3k
{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.
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
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 |
| 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:
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.
| 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 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.
| 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_dirparam 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 Testspends 24 min instead of 16min.I enable
random_config_dirfor the tests which useaz configoraz 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.