-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
[Microsoft.App] Advanced Ingress ACA - 2024-10-02-preview #30862
[Microsoft.App] Advanced Ingress ACA - 2024-10-02-preview #30862
Conversation
Next Steps to Merge✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge. |
Generated ApiView
|
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
"type": "object", | ||
"allOf": [ | ||
{ | ||
"$ref": "../../../../../common-types/resource-management/v5/types.json#/definitions/TrackedResource" |
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.
Why choose tracked vs proxy?
https://armwiki.azurewebsites.net/rp_onboarding/tracked_vs_proxy_resources.html
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.
Answered in comment below
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.
Confirm if proxy resource type makes more sense
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.
thank you for bringing this up, this is a good point! We will use ProxyResource. Changed.
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
...anager/Microsoft.App/preview/2024-10-02-preview/examples/HttpRouteConfig_CreateOrUpdate.json
Outdated
Show resolved
Hide resolved
...anager/Microsoft.App/preview/2024-10-02-preview/examples/HttpRouteConfig_CreateOrUpdate.json
Show resolved
Hide resolved
...anager/Microsoft.App/preview/2024-10-02-preview/examples/HttpRouteConfig_CreateOrUpdate.json
Outdated
Show resolved
Hide resolved
...anager/Microsoft.App/preview/2024-10-02-preview/examples/HttpRouteConfig_CreateOrUpdate.json
Outdated
Show resolved
Hide resolved
} | ||
} | ||
}, | ||
"delete": { |
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.
is it a long running operation?
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.
no, we are just deleting the crd directly from cluster and thats it. no other dependency resources need deletion.
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.
Confirm with Chris if resource creation happens async and will make this async.
Also to handle transient issues with managed cluster incase api-server availablity/throttling
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 will keep this as non long running
cc: @Tratcher
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
} | ||
} | ||
}, | ||
"201": { |
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 it is not long running operation, you should never return 201. Please remove 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.
i removed 201 and now it is failing lint validation
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 still need to adhere to synchronous running operation based on the source code of the lintDiff validation check
Hence, we need to add a 201 for non long runnning opps as well
https://github.com/Azure/azure-openapi-validator/blob/c26c69a9a4bc2c36a98752bb7dd89a7292e4a39a/packages/rulesets/src/spectral/functions/put-response-codes.ts#L39
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.
picking a random spec from another resource provider, they have one of their PUT
that has 201
responses.
https://github.com/tdaroly/azure-rest-api-specs/blob/tdarolywala/advanced-ingress-routing-2024-10-02-preview/specification/vi/resource-manager/Microsoft.VideoIndexer/preview/2024-06-01-preview/vi.json
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 lint error is for patch and the example file you update "HttpConfig_CreateOrUpdate" is for put. They are not related. Please remove 201 for HttpConfig_CreateOrUpdate. If there is lint error occured, please also fix as mentioned. If you think the lint error doesn't make sense, we can ask ARM for exception.
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.
@Juliehzl i have fixed the lint error for PATCH.
I am now referring to the other error pertaining to 201
status codes for non long running operations. It seems that ARM has added a validation that required 201
for those as well. You can refer to those here:
https://github.com/Azure/azure-openapi-validator/blob/c26c69a9a4bc2c36a98752bb7dd89a7292e4a39a/packages/rulesets/src/spectral/functions/put-response-codes.ts#L39
Do we not want to support that? if so, yes i agree with you that we should ask ARM for exception.
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 see. If it is treated as valid for arm, then I am fine with it. Thanks!
...anager/Microsoft.App/preview/2024-10-02-preview/examples/HttpRouteConfig_CreateOrUpdate.json
Outdated
Show resolved
Hide resolved
...anager/Microsoft.App/preview/2024-10-02-preview/examples/HttpRouteConfig_CreateOrUpdate.json
Outdated
Show resolved
Hide resolved
...anager/Microsoft.App/preview/2024-10-02-preview/examples/HttpRouteConfig_CreateOrUpdate.json
Outdated
Show resolved
Hide resolved
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
...ification/app/resource-manager/Microsoft.App/preview/2024-10-02-preview/HttpRouteConfig.json
Outdated
Show resolved
Hide resolved
…App` && `revision` under targets
…olywala/advanced-ingress-routing-2024-10-02-preview
...oft.App/preview/2024-10-02-preview/examples/HttpRouteConfig_CreateOrUpdatePathSepPrefix.json
Outdated
Show resolved
Hide resolved
…' of https://github.com/tdaroly/azure-rest-api-specs into tdarolywala/advanced-ingress-routing-2024-10-02-preview
…olywala/advanced-ingress-routing-2024-10-02-preview
…olywala/advanced-ingress-routing-2024-10-02-preview
Head branch was pushed to by a user without write access
…olywala/advanced-ingress-routing-2024-10-02-preview
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.
Added suggestions
…olywala/advanced-ingress-routing-2024-10-02-preview
/pr RequestMerge |
37712e1
into
Azure:release-app-Microsoft.App-2024-10-02-preview
ARM (Control Plane) API Specification Update Pull Request
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!
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
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.