Skip to content
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

Merged
merged 5 commits into from
Jul 26, 2023

Conversation

shenglol
Copy link
Contributor

@shenglol shenglol commented Jun 30, 2023

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 own DummyCli instance with random_config_dir set to True.

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.

@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Jun 30, 2023

️✔️AzureCLI-FullTest
️✔️acr
️✔️2020-09-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9
️✔️acs
️✔️2020-09-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9
️✔️advisor
️✔️latest
️✔️3.10
️✔️3.9
️✔️ams
️✔️latest
️✔️3.10
️✔️3.9
️✔️apim
️✔️latest
️✔️3.10
️✔️3.9
️✔️appconfig
️✔️latest
️✔️3.10
️✔️3.9
️✔️appservice
️✔️latest
️✔️3.10
️✔️3.9
️✔️aro
️✔️latest
️✔️3.10
️✔️3.9
️✔️backup
️✔️latest
️✔️3.10
️✔️3.9
️✔️batch
️✔️latest
️✔️3.10
️✔️3.9
️✔️batchai
️✔️latest
️✔️3.10
️✔️3.9
️✔️billing
️✔️latest
️✔️3.10
️✔️3.9
️✔️botservice
️✔️latest
️✔️3.10
️✔️3.9
️✔️cdn
️✔️latest
️✔️3.10
️✔️3.9
️✔️cloud
️✔️latest
️✔️3.10
️✔️3.9
️✔️cognitiveservices
️✔️latest
️✔️3.10
️✔️3.9
️✔️config
️✔️latest
️✔️3.10
️✔️3.9
️✔️configure
️✔️latest
️✔️3.10
️✔️3.9
️✔️consumption
️✔️latest
️✔️3.10
️✔️3.9
️✔️container
️✔️latest
️✔️3.10
️✔️3.9
️✔️core
️✔️2018-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9
️✔️cosmosdb
️✔️latest
️✔️3.10
️✔️3.9
️✔️databoxedge
️✔️2019-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9
️✔️dla
️✔️latest
️✔️3.10
️✔️3.9
️✔️dls
️✔️latest
️✔️3.10
️✔️3.9
️✔️dms
️✔️latest
️✔️3.10
️✔️3.9
️✔️eventgrid
️✔️latest
️✔️3.10
️✔️3.9
️✔️eventhubs
️✔️latest
️✔️3.10
️✔️3.9
️✔️feedback
️✔️latest
️✔️3.10
️✔️3.9
️✔️find
️✔️latest
️✔️3.10
️✔️3.9
️✔️hdinsight
️✔️latest
️✔️3.10
️✔️3.9
️✔️identity
️✔️latest
️✔️3.10
️✔️3.9
️✔️iot
️✔️2019-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9
️✔️keyvault
️✔️2018-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9
️✔️kusto
️✔️latest
️✔️3.10
️✔️3.9
️✔️lab
️✔️latest
️✔️3.10
️✔️3.9
️✔️managedservices
️✔️latest
️✔️3.10
️✔️3.9
️✔️maps
️✔️latest
️✔️3.10
️✔️3.9
️✔️marketplaceordering
️✔️latest
️✔️3.10
️✔️3.9
️✔️monitor
️✔️latest
️✔️3.10
️✔️3.9
️✔️mysql
️✔️latest
️✔️3.10
️✔️3.9
️✔️netappfiles
️✔️latest
️✔️3.10
️✔️3.9
️✔️network
️✔️2018-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9
️✔️policyinsights
️✔️latest
️✔️3.10
️✔️3.9
️✔️privatedns
️✔️latest
️✔️3.10
️✔️3.9
️✔️profile
️✔️latest
️✔️3.10
️✔️3.9
️✔️rdbms
️✔️latest
️✔️3.10
️✔️3.9
️✔️redis
️✔️latest
️✔️3.10
️✔️3.9
️✔️relay
️✔️latest
️✔️3.10
️✔️3.9
️✔️resource
️✔️2018-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9
️✔️role
️✔️latest
️✔️3.10
️✔️3.9
️✔️search
️✔️latest
️✔️3.10
️✔️3.9
️✔️security
️✔️latest
️✔️3.10
️✔️3.9
️✔️servicebus
️✔️latest
️✔️3.10
️✔️3.9
️✔️serviceconnector
️✔️latest
️✔️3.10
️✔️3.9
️✔️servicefabric
️✔️latest
️✔️3.10
️✔️3.9
️✔️signalr
️✔️latest
️✔️3.10
️✔️3.9
️✔️sql
️✔️latest
️✔️3.10
️✔️3.9
️✔️sqlvm
️✔️latest
️✔️3.10
️✔️3.9
️✔️storage
️✔️2018-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9
️✔️synapse
️✔️latest
️✔️3.10
️✔️3.9
️✔️telemetry
️✔️2018-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9
️✔️util
️✔️latest
️✔️3.10
️✔️3.9
️✔️vm
️✔️2018-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.10
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.10
️✔️3.9
️✔️latest
️✔️3.10
️✔️3.9

@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Jun 30, 2023

️✔️AzureCLI-BreakingChangeTest
️✔️Non Breaking Changes

@ghost ghost added the Auto-Assign Auto assign by bot label Jun 30, 2023
@yonzhan
Copy link
Collaborator

yonzhan commented Jun 30, 2023

ARM

@ghost ghost requested a review from yonzhan June 30, 2023 21:57
@ghost ghost assigned zhoxing-ms Jun 30, 2023
@ghost ghost added the ARM az resource/group/lock/tag/deployment/policy/managementapp/account management-group label Jun 30, 2023
@ghost ghost requested review from jiasli, wangzelin007 and bebound June 30, 2023 21:58
@ghost ghost assigned bebound Jun 30, 2023
@ghost ghost added the Packaging label Jun 30, 2023
@shenglol
Copy link
Contributor Author

@jiasli @zhoxing-ms Could you take a look?

@zhoxing-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@shenglol
Copy link
Contributor Author

shenglol commented Jul 3, 2023

Well, looks like azure-cli-extensions now has a dependency on semver (see https://github.com/Azure/azure-cli-extensions/blob/0b2cbaab7ecd11a8cd99eb5d455045abb22ee9c9/src/fleet/azext_fleet/_validators.py#L5), which causes the failure in CI. Should I add the semver dependency back in the requirements files?

@zhoxing-ms
Copy link
Contributor

Well, looks like azure-cli-extensions now has a dependency on semver (see https://github.com/Azure/azure-cli-extensions/blob/0b2cbaab7ecd11a8cd99eb5d455045abb22ee9c9/src/fleet/azext_fleet/_validators.py#L5), which causes the failure in CI. Should I add the semver dependency back in the requirements files?

Okay, this can avoid causing breaking changes to other dependent places

@shenglol shenglol changed the title {ARM} Replace semver with packaging.version {ARM} Fix race condition in bicep unit tests Jul 17, 2023
@shenglol
Copy link
Contributor Author

Well, looks like azure-cli-extensions now has a dependency on semver (see https://github.com/Azure/azure-cli-extensions/blob/0b2cbaab7ecd11a8cd99eb5d455045abb22ee9c9/src/fleet/azext_fleet/_validators.py#L5), which causes the failure in CI. Should I add the semver dependency back in the requirements files?

Okay, this can avoid causing breaking changes to other dependent places

Since we cannot remove the semver dependency, I restored the _bicep.py to use semver instead of package.versioning. In the future, if we start to publish pre-releases / private versions for Bicep, the version number may not be compliant with PEP 440.

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Jul 19, 2023

@shenglol Actually, I think we can skip removing semver from setup.py and requirements.py3.xxx in order not to affect other modules/extensions. However, to ensure the consistency in version management, it is still recommended that you use package.versioning instead of semver

@zhoxing-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@shenglol
Copy link
Contributor Author

@shenglol Shenglong Li FTE Actually, I think we can skip removing semver from setup.py and requirements.py3.xxx in order not to affect other modules/extensions. However, to ensure the consistency in version management, it is still recommended that you use package.versioning instead of semver

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.

@zhoxing-ms
Copy link
Contributor

@jiasli Could you please help review this PR?

@zhoxing-ms zhoxing-ms merged commit ad03a72 into Azure:dev Jul 26, 2023
54 checks passed
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
Copy link
Member

@jiasli jiasli Jul 26, 2023

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.

@jiasli jiasli mentioned this pull request Aug 4, 2023
3 tasks
avgale pushed a commit to avgale/azure-cli that referenced this pull request Aug 24, 2023
Comment on lines +25 to +26
def setUp(self):
self.cli_ctx = DummyCli(random_config_dir=True)
Copy link
Member

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:

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

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARM az resource/group/lock/tag/deployment/policy/managementapp/account management-group Auto-Assign Auto assign by bot Packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants