Skip to content

Conversation

@shenglol
Copy link
Contributor

@shenglol shenglol commented Feb 8, 2021

Description

The PR integrates Bicep CLI so that the ARM template deployment commands can deploy Bicep files.

Testing Guide

az deployment group create -g <rg> -f storage_account.bicep

History Notes

[ARM] az deployment group/sub/mg/tenant validate/create/what-if: Add support for Bicep files
[ARM] az bicep install: New command for installing Bicep CLI
[ARM] az bicep upgrade: New command for upgrading Bicep CLI
[ARM] az bicep build: New command for building Bicep files
[ARM] az bicep version: New command for showing the current installed version of Bicep CLI
[ARM] az bicep list-versions: New command for showing the available Bicep CLI versions


This checklist is used to make sure that common guidelines for a pull request are followed.

@yonzhan yonzhan added this to the S183 - For Ignite milestone Feb 8, 2021
@yonzhan yonzhan requested a review from jsntcy February 8, 2021 23:24
@yonzhan
Copy link
Collaborator

yonzhan commented Feb 8, 2021

ARM

@zhoxing-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@shenglol
Copy link
Contributor Author

shenglol commented Feb 9, 2021

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 16857 in repo Azure/azure-cli

@zhoxing-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@zhoxing-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@shenglol
Copy link
Contributor Author

Hi @zhoxing-ms @Juliehzl could you take a look at this PR? We are hoping to get this merged to catch the release for ignite.


print(f'Successfully installed Bicep CLI to "{installation_path}".')
except IOError as err:
raise CLIError(f"Error while attempting to download Bicep CLI: {err}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change those CLIError to some more specific error?
Please refer to azure.cli.core.azclierror

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed them to more specific error types.

@zhoxing-ms
Copy link
Contributor

[ARM] az deployment group/sub/mg/tenant validate/create/what-if: Add support for Bicep files

Could we add some tests for this feature?

@shenglol
Copy link
Contributor Author

shenglol commented Feb 18, 2021

[ARM] az deployment group/sub/mg/tenant validate/create/what-if: Add support for Bicep files

Could we add some tests for this feature?

@zhoxing-ms I would like to hear your thoughts on how to properly test them. I added some e2e tests in e89fc1e, but I had to revert them because I realized that it won't pass CI. Since I'm using a Windows machine, the recording files will contain the binary data of the Bicep CLI exe, which cannot be invoked on Linux and macOS. The other thing that worries me is that with the Bicep CLI binary data significantly increases the size of recording files (60MB+ each), and I feel like I should not commit something that big...

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Feb 19, 2021

I would like to hear your thoughts on how to properly test them. I added some e2e tests in e89fc1e, but I had to revert them because I realized that it won't pass CI. Since I'm using a Windows machine, the recording files will contain the binary data of the Bicep CLI exe, which cannot be invoked on Linux and macOS. The other thing that worries me is that with the Bicep CLI binary data significantly increases the size of recording files (60MB+ each), and I feel like I should not commit something that big...

@shenglol OK, how about changing them to live test? How long do these tests take in live mode?

@shenglol
Copy link
Contributor Author

I would like to hear your thoughts on how to properly test them. I added some e2e tests in e89fc1e, but I had to revert them because I realized that it won't pass CI. Since I'm using a Windows machine, the recording files will contain the binary data of the Bicep CLI exe, which cannot be invoked on Linux and macOS. The other thing that worries me is that with the Bicep CLI binary data significantly increases the size of recording files (60MB+ each), and I feel like I should not commit something that big...

@shenglol OK, how about changing them to live test? How long do these tests take in live mode?

Changing them to live tests means that recording files won't be generated, right? If yes I'd be happy to do so. The tests takes about about 10 minutes to complete on my machine.

@zhoxing-ms
Copy link
Contributor

Changing them to live tests means that recording files won't be generated, right? If yes I'd be happy to do so. The tests takes about about 10 minutes to complete on my machine.

These record files are too large, and the binary data contained in the record files will be different when recording on different system platforms, right?
If so, I personally think live test is more appropriate

@shenglol
Copy link
Contributor Author

Changing them to live tests means that recording files won't be generated, right? If yes I'd be happy to do so. The tests takes about about 10 minutes to complete on my machine.

These record files are too large, and the binary data contained in the record files will be different when recording on different system platforms, right?
If so, I personally think live test is more appropriate

Got it. I'll add back the tests and change them to live tests.

@shenglol shenglol requested a review from zhoxing-ms February 22, 2021 18:27
@zhoxing-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).


installed_version = _get_bicep_installed_version(installation_path)
target_version = _extract_semver(release_tag)
if installed_version and target_version and semver.compare(installed_version, target_version) == 0:
Copy link
Member

@jiasli jiasli May 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shenglol, if bicep has no fancy versioning schema and complies with PEP 440, can semver be replaced by packaging.version? packaging.version is used by az extension (#17667):

def ext_compat_with_cli(azext_metadata):
from azure.cli.core import __version__ as core_version
from packaging.version import parse
is_compatible, min_required, max_required = (True, None, None)
if azext_metadata:
min_required = azext_metadata.get(EXT_METADATA_MINCLICOREVERSION)
max_required = azext_metadata.get(EXT_METADATA_MAXCLICOREVERSION)
parsed_cli_version = parse(core_version)
if min_required and parsed_cli_version < parse(min_required):
is_compatible = False
elif max_required and parsed_cli_version > parse(max_required):
is_compatible = False

This will help us reduce Azure CLI’s packaging maintenance cost (#26523).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants