-
Notifications
You must be signed in to change notification settings - Fork 3.3k
{Role} Bump role_definitions API version to 2022-05-01-preview
#26577
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
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Role |
|
This needs to be solved by #26475. A temporary workaround is to remove |
|
|
@yanzhudd will help take a look at this test issue~ |
Hi @jiasli if self.is_live:
time.sleep(30) |
|
#27222 introduces merge conflicts and makes recordings for |
f99363b to
f8c5501
Compare
| self.cmd('sig image-version delete -g {rg} --gallery-name {gallery} --gallery-image-definition {image} --gallery-image-version {version}') | ||
| time.sleep(60) # service end latency | ||
| self.cmd('sig image-definition delete -g {rg} --gallery-name {gallery} --gallery-image-definition {image}') | ||
| time.sleep(60) |
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.
An extra sleep is added to fix #26577 (comment) according to #26577 (comment).
time.sleep is patched during playback mode, so there is no need to add if self.is_live: check:
azure-cli/src/azure-cli-testsdk/azure/cli/testsdk/scenario_tests/patches.py
Lines 9 to 13 in 30216d0
| def patch_time_sleep_api(unit_test): | |
| def _time_sleep_skip(*_): | |
| return | |
| mock_in_unit_test(unit_test, 'time.sleep', _time_sleep_skip) |
|
|
||
| def __init__(self, *arg, **kwargs): | ||
| super().__init__(*arg, random_config_dir=True, **kwargs) | ||
| super().__init__(*arg, **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 needs to be removed according to #26577 (comment)
@bebound, should I revert it? If not, playback test may fail. If yes, live test will certainly fail, so other team member can't live run role module's tests.
I can also temporarily disable test_role_assignment_handle_conflicted_assignments to eliminate the root cause which writes to config file.
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.
My preferred solution is to merge #26475.
This is a good chance to test it works. It only affect test, even if it contains bugs, it will not affect user.
|
|
||
| @ResourceGroupPreparer(name_prefix='cli_role_assign') | ||
| @AllowLargeResponse() | ||
| def test_role_assignment_handle_conflicted_assignments(self, resource_group): |
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.
test_role_assignment_handle_conflicted_assignments is the only test that writes to config file. I moved it to a dedicated RoleAssignmentWithConfigScenarioTest so that other tests in RoleAssignmentScenarioTest won't need random_config_dir.
Related command
az roleDescription
Close #26429
Bump
role_definitionsAPI version to 2022-05-01-preview