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

2024-01-01-preview #7392

Merged
merged 21 commits into from
Apr 1, 2024
Merged

Conversation

jamesfan1
Copy link
Member

@jamesfan1 jamesfan1 commented Mar 19, 2024


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

Related command

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)
  • My extension version conforms to the Extension version schema

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

Copy link

azure-client-tools-bot-prd bot commented Mar 19, 2024

❌Azure CLI Extensions Breaking Change Test
❌nginx
rule cmd_name rule_message suggest_message
1007 - ParaRemove nginx deployment configuration analyze cmd nginx deployment configuration analyze removed parameter config please add back parameter config for cmd nginx deployment configuration analyze
⚠️ 1006 - ParaAdd nginx deployment configuration analyze cmd nginx deployment configuration analyze added parameter files
⚠️ 1006 - ParaAdd nginx deployment configuration analyze cmd nginx deployment configuration analyze added parameter package
⚠️ 1006 - ParaAdd nginx deployment configuration analyze cmd nginx deployment configuration analyze added parameter protected_files
⚠️ 1006 - ParaAdd nginx deployment configuration analyze cmd nginx deployment configuration analyze added parameter root_file
⚠️ 1001 - CmdAdd nginx deployment configuration update cmd nginx deployment configuration update added
⚠️ 1006 - ParaAdd nginx deployment create cmd nginx deployment create added parameter auto_upgrade_profile
⚠️ 1006 - ParaAdd nginx deployment update cmd nginx deployment update added parameter auto_upgrade_profile

Copy link

Hi @jamesfan1,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

@yonzhan
Copy link
Collaborator

yonzhan commented Mar 19, 2024

Thank you for your contribution! We will review the pull request and get back to you soon.

Copy link

github-actions bot commented Mar 19, 2024

⚠️ Suggestions

Module: nginx

  • Update version to 2.0.0b1 in setup.py

Notes

  • Stable/preview tag is inherited from last release. If needed, please add stable/preview label to modify it.
  • Major/minor/patch/pre increment of version number is calculated by pull request code changes automatically. If needed, please add major/minor/patch/pre label to adjust it.
  • For more info about extension versioning, please refer to Extension version schema


capacity = cls._args_schema.scaling_properties.profiles.Element.capacity
capacity.max = AAZIntArg(
options=["max"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how this looks like on Azure CLI but "min" and "max" are not good names as they don't tell the user much about what the min and max are about or applied to.

Copy link
Member Author

@jamesfan1 jamesfan1 Mar 27, 2024

Choose a reason for hiding this comment

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

--scaling-properties profiles=[{name:default,capacity:{min:10,max:30}}]
perhaps if we add this sample in the help command it should explain it?

@@ -143,6 +148,13 @@ def _build_arguments_schema(cls, *args, **kwargs):
help={"short-summary": "Optional: Preferred communication email", "long-summary": "Usage --user-profile [email protected]"},
)

auto_upgrade_profile = cls._args_schema.auto_upgrade_profile
auto_upgrade_profile.upgrade_channel = AAZStrArg(
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way to add some "help" around valid values? If so, can you please add preview and stable

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@jamesfan1
Copy link
Member Author

jamesfan1 commented Mar 27, 2024

tests are passing locally, not sure why it doesnt pass in the pipeline. All new command tested manually and succeeding

@yonzhan
Copy link
Collaborator

yonzhan commented Mar 27, 2024

Please fix CI issues

@AllyW AllyW added the preview release extension module as preview label Mar 28, 2024
@AllyW
Copy link
Member

AllyW commented Mar 28, 2024

⚠️ Suggestions

Module: nginx

  • Update version to 2.0.0b1 in setup.py

Notes

  • Stable/preview tag is inherited from last release. If needed, please add stable/preview label to modify it.
  • Major/minor/patch/pre increment of version number is calculated by pull request code changes automatically. If needed, please add major/minor/patch/pre label to adjust it.
  • For more info about extension versioning, please refer to Extension version schema

@jamesfan1 there is a breaking change in this pr which deleted a parameter. If it's unintentional, please fix it, if it is intented, that's ok.

Another problem is the version number, the last released version is 1.0.0, since there is a breaking change, so the next release version would be 2.0.0. And if you plan to release a preview version, it should be 2.0.0b1. 1.0.0b1 is smaller than 1.0.0 and we should not release an older version than current one.

For version details, please refer to our doc here

@@ -3,6 +3,10 @@
Release History
===============

1.0.0b1
Copy link
Member

Choose a reason for hiding this comment

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

please correct version number

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, thank you

@jamesfan1
Copy link
Member Author

@AllyW

image

I am unsure what is wrong for
Azure.azure-cli-extensions Integration test

it is passing locally, and generated a recording file

however, on ci it's failing with

ERROR cli.azure.cli.core.azclierror:azlogging.py:212 Can't overwrite existing cassette ('/mnt/vss/_work/1/s/src/nginx/azext_nginx/tests/latest/recordings/test_deployment_cert_config.yaml') in your current record mode ('once').

on this line, but this line runs fine locally
public_ip = self.cmd('network public-ip create --resource-group {rg} --name {public_ip_name} --version IPv4 --sku Standard --zone 2').get_output_in_json()

have you seen such error?

@@ -1,3 +1,3 @@
{
"azext.minCliCoreVersion": "2.55.0"
"azext.minCliCoreVersion": "2.58.0"
Copy link
Member

Choose a reason for hiding this comment

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

Please set azext.isPreview to true as you will release it as preview.

@jamesfan1
Copy link
Member Author

added the tag, please merge when possible. Thank you all for the help!!
@jsntcy @AllyW

@wangzelin007 wangzelin007 merged commit 8250be4 into Azure:main Apr 1, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-cal-version preview release extension module as preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants