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

VMware LRO fixes for rotate-vcenter-password, rotate-nsxt-password, and restrict-movement #8462

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jonathanhe-msft
Copy link
Contributor

@jonathanhe-msft jonathanhe-msft commented Feb 8, 2025


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

Related command

az vmware private-cloud rotate-vcenter-password, az vmware private-cloud rotate-nsxt-password, and az vmware vm restrict-movement

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 Feb 8, 2025

❌Azure CLI Extensions Breaking Change Test
❌vmware
rule cmd_name rule_message suggest_message
1007 - ParaRemove vmware private-cloud delete-identity-source cmd vmware private-cloud delete-identity-source removed parameter alias please add back parameter alias for cmd vmware private-cloud delete-identity-source
1007 - ParaRemove vmware private-cloud delete-identity-source cmd vmware private-cloud delete-identity-source removed parameter domain please add back parameter domain for cmd vmware private-cloud delete-identity-source
1007 - ParaRemove vmware private-cloud deleteidentitysource cmd vmware private-cloud deleteidentitysource removed parameter alias please add back parameter alias for cmd vmware private-cloud deleteidentitysource
1007 - ParaRemove vmware private-cloud deleteidentitysource cmd vmware private-cloud deleteidentitysource removed parameter domain please add back parameter domain for cmd vmware private-cloud deleteidentitysource

Copy link

Hi @jonathanhe-msft,
Since the current milestone time is less than 7 days, this pr will be reviewed in the next milestone.

Copy link

Hi @jonathanhe-msft,
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 Feb 8, 2025

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

Copy link

github-actions bot commented Feb 8, 2025

The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR.

Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions).
After that please run the following commands to enable git hooks:

pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>

Copy link

github-actions bot commented Feb 8, 2025

CodeGen Tools Feedback Collection

Thank you for using our CodeGen tool. We value your feedback, and we would like to know how we can improve our product. Please take a few minutes to fill our codegen survey

Copy link

github-actions bot commented Feb 8, 2025

@yonzhan yonzhan requested a review from yanzhudd February 8, 2025 00:56
@yonzhan yonzhan removed the request for review from Juliehzl February 8, 2025 00:59
@jonathanhe-msft jonathanhe-msft changed the title Jonathanhe/lro fix VMware LRO fixes for rotate-vcenter-password, rotate-nsxt-password, and restrict-movement Feb 10, 2025
Comment on lines +201 to +203
properties.endpoints = AAZObjectType(
flags={"read_only": True},
)
Copy link
Contributor

@zhoxing-ms zhoxing-ms Feb 11, 2025

Choose a reason for hiding this comment

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

I see that many properties have been marked as read-only. May I ask what is the reason? Is this caused by the Swagger change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not due to swagger change as swagger has had these values since it was created.

I believe that it's due to the python version upgrade and aaz-dev-tools upgrades. I have double checked and the properties do exist in swagger, so there could have been upgrades to code-gen tool that now capture these property flags.

@github-actions github-actions bot added the release-version-block Updates do not qualify release version rules. NOTE: please do not edit it manually. label Feb 19, 2025
@jonathanhe-msft
Copy link
Contributor Author

When generating the client using aaz-dev, I noticed that new line characters in confirmation texts are escaped with a double \ eg \\n. When I run these commands, the text is formatted incorrectly (does not display as a new line, instead displays the now escaped new line characters. I modified the text manually to revert it back to the new lines. This can be seen in the second to last commit.

Can I get some confirmation that this is incorrectly generated and \n should not be escaped?

@AllyW AllyW added the minor release extension module with version minor upgraded label Feb 20, 2025
@github-actions github-actions bot removed the release-version-block Updates do not qualify release version rules. NOTE: please do not edit it manually. label Feb 20, 2025
@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Feb 27, 2025

When generating the client using aaz-dev, I noticed that new line characters in confirmation texts are escaped with a double \ eg \n. When I run these commands, the text is formatted incorrectly (does not display as a new line, instead displays the now escaped new line characters. I modified the text manually to revert it back to the new lines. This can be seen in the second to last commit. Can I get some confirmation that this is incorrectly generated and \n should not be escaped?

@kairu-ms This question seems to be related to aaz-dev~ If so, could you please take a look at this Code Gen related question?

@zhoxing-ms
Copy link
Contributor

Could you please resolve these CI issues?

@kairu-ms
Copy link
Contributor

When generating the client using aaz-dev, I noticed that new line characters in confirmation texts are escaped with a double \ eg \\n. When I run these commands, the text is formatted incorrectly (does not display as a new line, instead displays the now escaped new line characters. I modified the text manually to revert it back to the new lines. This can be seen in the second to last commit.

Can I get some confirmation that this is incorrectly generated and \n should not be escaped?

There’s no need to add \n in help. I think you can sample remove them

@jonathanhe-msft
Copy link
Contributor Author

When generating the client using aaz-dev, I noticed that new line characters in confirmation texts are escaped with a double \ eg \\n. When I run these commands, the text is formatted incorrectly (does not display as a new line, instead displays the now escaped new line characters. I modified the text manually to revert it back to the new lines. This can be seen in the second to last commit.
Can I get some confirmation that this is incorrectly generated and \n should not be escaped?

There’s no need to add \n in help. I think you can sample remove them

We want to keep the line spacing though. It's a long message and is much more readable when split up

{4C90BDD0-5D4D-4036-B996-3A91C7C8794E}

@jonathanhe-msft
Copy link
Contributor Author

Could you please resolve these CI issues?

I resolved the scan issues by removing what it was calling out (even though it doesn't seem like it's actually a credential).

For the style issues, I removed the unused arguments. For the other errors about Access to a protected member _registered of a client class, can they be addressed in a separate PR? I see that it is an optional check for now and the problem was introduced when Kai helped us convert our extension to use the code-gen tools. I'd like to get this merged first as it is associated with a CRI.

@kairu-ms kairu-ms closed this Feb 28, 2025
@kairu-ms kairu-ms reopened this Feb 28, 2025
@necusjz
Copy link
Member

necusjz commented Feb 28, 2025

When generating the client using aaz-dev, I noticed that new line characters in confirmation texts are escaped with a double \ eg \\n. When I run these commands, the text is formatted incorrectly (does not display as a new line, instead displays the now escaped new line characters. I modified the text manually to revert it back to the new lines. This can be seen in the second to last commit.
Can I get some confirmation that this is incorrectly generated and \n should not be escaped?

There’s no need to add \n in help. I think you can sample remove them

We want to keep the line spacing though. It's a long message and is much more readable when split up

{4C90BDD0-5D4D-4036-B996-3A91C7C8794E}

@jonathanhe-msft have you tried add a new line by \n\n? E.g.,

image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor release extension module with version minor upgraded
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants