-
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
{ARM} Fix race condition in bicep unit tests #26797
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
ARM |
@jiasli @zhoxing-ms Could you take a look? |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Well, looks like azure-cli-extensions now has a dependency on |
Okay, this can avoid causing breaking changes to other dependent places |
semver
with packaging.version
Since we cannot remove the |
@shenglol Actually, I think we can skip removing |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
To clarify, Bicep CLI is technically using semantic versioning. The version is set by Nerdbank.GitVersioning, which produces semver-compatible versions. Currently, for public releases, the version is always in the format of MAJOR.MINOR.PATCH and is also compatible with PEP 440. However, in the future, we might want to publish private or pre-release versions that may contain suffixes (e.g., commit hashes) that are not necessarily compatible with PEP 440. |
@jiasli Could you please help review this PR? |
semver_match = re.search(_semver_pattern, text) | ||
return semver_match.group(0) if semver_match else None | ||
return semver.VersionInfo.parse(semver_match.group(0)) if semver_match else None |
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 semver
is indeed required, we should consider reviving #26523.
def setUp(self): | ||
self.cli_ctx = DummyCli(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.
This contradicts the convention used in #25689 that random_config_dir=True
is set in super().__init__
invocation:
azure-cli/src/azure-cli/azure/cli/command_modules/config/tests/latest/test_config.py
Lines 16 to 17 in b223dd9
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.
Its superclass is unittest.TestCase
, which does not has random_config_dir
param.
Related command
az bicep *
Description
This main purpose of the PR is to reduce Azure CLI’s packaging maintenance overheads based on @jiasli's feedback #16857 (comment).The PR also fixes race conditions in
test_resource_bicep.py
by having each test uses their ownDummyCli
instance withrandom_config_dir
set toTrue
.Testing Guide
The existing tests should be sufficient the cover the changes.
History Notes
This 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.