-
Notifications
You must be signed in to change notification settings - Fork 5.3k
changing request byte from string and descoping submit endpoints #34293
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
base: main
Are you sure you want to change the base?
changing request byte from string and descoping submit endpoints #34293
Conversation
Next Steps to MergeNext steps that must be taken to merge this PR:
|
PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment. |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews
|
…hardikginwala/azure-rest-api-specs into dev/hginwala/removeSubmitroutes
/azp run |
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
@markcowl requesting suppression exception for swagger LintDiff since this is a valid usage for this endpoint on our service as discussed on teams chat |
Did it get consumed anywhere else though? You may need to follow up and ensure any inaccurate SDKs / docs are updated or removed!! |
@doc("resourceOverrides, properties per resource that needs to be overwritted from baseProfile") | ||
resourceOverrides?: Array<string>; | ||
#suppress "@azure-tools/typespec-azure-resource-manager/missing-x-ms-identifiers" "reviewed this with ARM, we are a pass through to compute RP and doing any property specific validation, compute RP will do that." | ||
@doc("resourceOverrides, properties per resource that needs to be overwritten from baseProfile") |
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.
we had a API modeling session for this last month 04/03 at 12:30 EST, where we described use case for this API and necessity to go with schema free json approach here
this API is primarily targeted for 1P apps and RPs (like Fleet) that are today running into issues while creating single instance VMs through compute RP at scale, because of throttling and transient issues.
These services are going to leverage our RP as pass through, we are going to add value by offering scheduling and handling transient errors, end users will be passing VM profiles for which they want to create virtual machines.
Our service will merge Baseprofile with overrides for VM properties, and pass that merged profile to compute RP create VM endpoint, clients will pass their targeted API version and we will call compute RP for that version, so type checking against schema will be done by compute RP as they are doing today. End users who are creating just less than 1000 VMs won't typically be using this RP but even for them we will update RP documentation on learn.microsoft with examples on how to consume.
But our target audience for foreseeable future are 1P apps like CPC, devbox, AVD, RPs like Fleet and AKS, who are already doing this programmatically.
@@ -137,11 +137,13 @@ model Resources { | |||
@added(Microsoft.ComputeSchedule.Versions.`2025-05-01`) | |||
@doc("Resource creation data model") | |||
model ResourceProvisionPayload { | |||
#suppress "@azure-tools/typespec-azure-resource-manager/arm-no-record" "reviewed this with ARM, we are a pass through to compute RP and doing any property specific validation, compute RP will do that." | |||
@doc("baseProfile, Resource properties that common across all resources ") |
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.
same as above
we had a API modeling session for this last month 04/03 at 12:30 EST, where we described use case for this API and necessity to go with schema free json approach here
this API is primarily targeted for 1P apps and RPs (like Fleet) that are today running into issues while creating single instance VMs through compute RP at scale, because of throttling and transient issues.
These services are going to leverage our RP as pass through, we are going to add value by offering scheduling and handling transient errors, end users will be passing VM profiles for which they want to create virtual machines.
Our service will merge Baseprofile with overrides for VM properties, and pass that merged profile to compute RP create VM endpoint, clients will pass their targeted API version and we will call compute RP for that version, so type checking against schema will be done by compute RP as they are doing today. End users who are creating just less than 1000 VMs won't typically be using this RP but even for them we will update RP documentation on learn.microsoft with examples on how to consume.
But our target audience for foreseeable future are 1P apps like CPC, devbox, AVD, RPs like Fleet and AKS, who are already doing this programmatically.
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.
As below
Sorry, what I mean is, is the description you are giving of your API, really going to be obvious enough to the API consumers, who are reading your doc?
@@ -137,11 +137,13 @@ model Resources { | |||
@added(Microsoft.ComputeSchedule.Versions.`2025-05-01`) | |||
@doc("Resource creation data model") |
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.
am open for updating description but I feel best way to address this will be with an example and documentation on learn.microsoft, our early adopters for this API are other Azure services and we have involved them early in design phase, so they are aware, and we have shared sample request/response objects with them.
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.
Sorry, what I mean is, is the description you are giving of your API, really going to be obvious enough to the API consumers, who are reading your doc?
For suppressions, sure, ARM reviewers are a target audience.
But also the main target audience of these specs is supposed to be the consumers, and they need to have some signposts telling them what schema they are actually allowed to use here.
@@ -521,6 +488,9 @@ model ExecuteDeleteRequest { | |||
|
|||
@doc("Forced delete resource item") | |||
forceDeletion?: boolean; | |||
|
|||
@doc("Microsoft.Compute API version to target when calling delete endpoint.") | |||
armApiVersion?: string; |
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.
agreed
"$ref": "./examples/Operations_List_MaximumSet_Gen.json" | ||
}, | ||
"Operations_List_MaximumSet_Gen - generated by [MaximumSet] rule - generated by [MaximumSet] rule - generated by [MinimumSet] rule": { | ||
"Operations_List_MaximumSet_Gen - generated by [MaximumSet] rule - generated by [MaximumSet] rule - generated by [MaximumSet] rule - generated by [MinimumSet] rule": { |
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.
@@ -71,10 +71,10 @@ | |||
} | |||
}, | |||
"x-ms-examples": { | |||
"Operations_List_MaximumSet_Gen - generated by [MaximumSet] rule - generated by [MaximumSet] rule - generated by [MaximumSet] rule": { | |||
"Operations_List_MaximumSet_Gen - generated by [MaximumSet] rule - generated by [MaximumSet] rule - generated by [MaximumSet] rule - generated by [MaximumSet] rule": { |
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.
also why does it keep getting longer every time you generate it(?)
No, this is not consumed through any other means, because backend service for supporting this version isn't out yet, moreover we are primarily going to work on this with Fleet for (Databricks and Snowflake) and we are supposed to start onboarding these mid June. |
@@ -15,7 +15,7 @@ | |||
"provider": "oxdxyfefyvtxexszpvt", |
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.
ARM (Control Plane) API Specification Update Pull Request for Changing request to allow JSON objects and descoping submit endpoints.
Tip
Overwhelmed by all this guidance? See the
Getting help
section at the bottom of this PR description.PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
Purpose of this PR
What's the purpose of this PR? Check the specific option that applies. This is mandatory!
This API version is still under development and we not releasing this to customer, during our internal testing we found that we have to make these changes before we share this with customers.
API version 2025-05-01 is not used by any customer yet.
Due diligence checklist
To merge this PR, you must go through the following checklist and confirm you understood
and followed the instructions by checking all the boxes:
ARM resource provider contract and
REST guidelines (estimated time: 4 hours).
I understand this is required before I can proceed to the diagram Step 2, "ARM API changes review", for this PR.
Additional information
Viewing API changes
removing submitCreate and submitDelete ,
for supporting our client needs we only need executeCreate and executeDelete endpoints.
we incorrectly specified type of baseProfile and resourceOverrides as string and array respectively, in our testing in canary we realized this isn't correct, we want to pass JSON object as input hence we are changing this to byte and array
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiView
comment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
suppressions guide to get approval.
Getting help
Purpose of this PR
andDue diligence checklist
.write access
per aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Merge
comment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.and https://aka.ms/ci-fix.
queued
state, please add a comment with contents/azp run
.This should result in a new comment denoting a
PR validation pipeline
has started and the checks should be updated after few minutes.