-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Update connectedmachine extension #2364
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
Update connectedmachine extension #2364
Conversation
add alias for auto_rev_minor_version
|
If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR: |
|
Majority code is generated, less manual written. so LGTM. |
|
Yeah the commits tell you what is generated and what is manually edited |
| short-summary: "The operation to create or update the extension." | ||
| examples: | ||
| - name: Create or Update a Machine Extension |
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.
I think we should remove or update here. @qiaozha can we do this in codegen automatically?
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.
Make sense. I filed an issue to track this Azure/autorest.az#553
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.
Suddenly realized the example name is from swagger example reference name. https://github.com/Azure/azure-rest-api-specs/blob/master/specification/hybridcompute/resource-manager/Microsoft.HybridCompute/stable/2020-08-02/HybridCompute.json#L90. Not sure we can change it as we use this to identify the example.
If you mean the short summary I think we can change it.
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 you check the example in the update command below, it's using the same name. It's better to distinguish them. Please evaluate further the feasibility of change as this is a common case in swagger.
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.
Let me know if there's something I need to do here
| with self.argument_context('connectedmachine machine-extension show') as c: | ||
| c.argument('resource_group_name', resource_group_name_type) | ||
| c.argument('machine_name', type=str, help='The name of the machine containing the extension.', id_part='name') | ||
| c.argument('name', type=str, help='The name of the machine extension.', id_part='child_name_1') |
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.
Add options_list=['--name', '-n'].
@qiaozha when the argument name is name, the options are not added?
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.
The code is generated. How would I do this in a generated way?
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.
Ive added the change manually in this PR and in Azure/azure-rest-api-specs#10795
| c.argument('resource_group_name', resource_group_name_type) | ||
| c.argument('machine_name', type=str, help='The name of the machine where the extension should be created or ' | ||
| 'updated.') | ||
| c.argument('name', type=str, help='The name of the machine extension.') |
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.
Add options_list=['--name', '-n'] for name arguments in all below commands as well.
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.
The code is generated. How would I do this in a generated way?
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.
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.
Ive added the change manually in this PR and in Azure/azure-rest-api-specs#10795
src/connectedmachine/azext_connectedmachine/tests/latest/test_connectedmachine_scenario.py
Show resolved
Hide resolved
Co-authored-by: Feng Zhou <[email protected]>
|
Awesome :) this should be ready to merge. |
This updates the connectedmachine extension to 2020-08-02 of the spec. In doing this I also changed the autogenerated tests to test more useful things.
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally?For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update
src/index.jsonautomatically.The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify
src/index.json.