-
Notifications
You must be signed in to change notification settings - Fork 2.1k
mgmt compute, support beginCreate with context for VirtualMachine #45597
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
Conversation
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
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.
Pull Request Overview
This PR adds support for beginning a VirtualMachine creation operation with a Context, propagates that context through async calls, and includes a test to verify context propagation.
- Introduced
beginCreate(Context)overload inVirtualMachineinterface and implementation - Propagated context via Reactor’s
contextWriteinVirtualMachineImpl - Added a unit test
canBeginCreateWithContextto validate context in HTTP pipeline policies
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| VirtualMachineOperationsTests.java | Added test canBeginCreateWithContext and support methods to inject and verify context through pipeline policies |
| VirtualMachine.java | Added beginCreate(Context) method signature and Javadoc |
| VirtualMachineImpl.java | Implemented beginCreate(Context) to propagate Reactor context to service calls and dependency tasks |
| pom.xml | Opened additional modules for reflective access in tests |
| assets.json | Updated asset tag reference |
| CHANGELOG.md | Documented the new beginCreate(Context) feature |
Comments suppressed due to low confidence (2)
sdk/resourcemanager/azure-resourcemanager-compute/src/main/java/com/azure/resourcemanager/compute/models/VirtualMachine.java:2200
- The Javadoc references
WithCreate#create(Context)which does not exist; this should link toWithCreate#beginCreate(Context).
* Please use {@link WithCreate#create(Context)} if virtual machine extensions is configured.
sdk/resourcemanager/azure-resourcemanager-compute/src/test/java/com/azure/resourcemanager/compute/VirtualMachineOperationsTests.java:2352
- [nitpick] Using reflection to access a private
HttpPipelineconstructor is brittle; consider using a supported builder or a pipeline customization hook instead of direct reflection.
Constructor<HttpPipeline> pipelineConstructor = HttpPipeline.class.getDeclaredConstructor(HttpClient.class, List.class, Tracer.class);
...mpute/src/main/java/com/azure/resourcemanager/compute/implementation/VirtualMachineImpl.java
Show resolved
Hide resolved
...r-compute/src/test/java/com/azure/resourcemanager/compute/VirtualMachineOperationsTests.java
Outdated
Show resolved
Hide resolved
...r-compute/src/test/java/com/azure/resourcemanager/compute/VirtualMachineOperationsTests.java
Outdated
Show resolved
Hide resolved
...r-compute/src/test/java/com/azure/resourcemanager/compute/VirtualMachineOperationsTests.java
Outdated
Show resolved
Hide resolved
...r-compute/src/test/java/com/azure/resourcemanager/compute/VirtualMachineOperationsTests.java
Show resolved
Hide resolved
weidongxu-microsoft
left a comment
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.
Code should be fine.
|
@XiaofeiCao Thank you for doing this! Is it possible to add
|
@sfc-gh-krank Yeah, we could. Wonder if you also need Also, what about |
|
We don't create disks separately. It's all part off ARM template. But we do plan to support individual disks management (create, snapshots etc...) in the future. So when you are touching these APIs now, it makes sense to add
|
|
@sfc-gh-krank Got it. Here's a list of methods we plan to support:
Let us know if you need more. /cc @weidongxu-microsoft |
Thanks @XiaofeiCao Here are some more methods we are using today on top of the ones you have listed Not sure if it's possible to add context to those list/get calls. It would be very helpful if they can get supported. |
@sfc-gh-hsong Sure, we'll add them. For
Do you mean |
|
Thanks @XiaofeiCao Below is our current implementation to begin a deployment given a template But yes, let's add |
|
We should already have Line 307 in 16da6bd
Would you help check again? |
Thanks @XiaofeiCao You are right, it's there for Can you please keep us posted once the PR is out so that we can follow up on the SDK version upgrade? Thanks |
|
@sfc-gh-hsong Sure, here's the PR: Feel free to leave any comments. |
|
Hi @sfc-gh-hsong , 2.52.0 has released with these features! |
|
Thanks @XiaofeiCao ! |
Description
customer request: https://stackoverflow.microsoft.com/questions/459001
co-related resources, e.g. resourceGroup, disks, networkInterface, will also share the Context.
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines