Azure Container Apps session data plane #30424
Conversation
| #suppress "@azure-tools/typespec-azure-core/use-standard-operations" "" | ||
| @doc("Execute code in a session.") | ||
| @route("/execute:executeCode") | ||
| executeCode is Foundations.Operation< |
There was a problem hiding this comment.
Looking at your Execution, it is still resource like. Please see playground to see how to model it and use the standard operation template.
| #suppress "@azure-tools/typespec-azure-core/use-standard-operations" "" | ||
| @doc("Get the content of the file.") | ||
| @route("/files/{name}/content") | ||
| getFileContent is Foundations.Operation< |
There was a problem hiding this comment.
This fits the ResourceAction perfectly. See playground
There was a problem hiding this comment.
Thank for the comments and sample code in playground, I am able to update the original GetExecutionResult to use standard operation.
But for execute code, the request body is model SessionCodeExecutionRequest and is different from the response SessionCodeExecutionResource, so LongRunningResourceCreateWithServiceProvidedName might not be a perfect fit in this case.
And for GetFileContent, using ResourceAction would let this Api become a POST operation rather than a GET, so I was't able to update to use ResourceAction.
Are there any suggestions for this scenario? Thanks!
There was a problem hiding this comment.
Actually the playground countains another example for the getContent using ResourceAction.
I pulled your PR and used following code and it produced same swagger
#suppress "@azure-tools/typespec-azure-core/verb-conflict" "GET allowed on Action"
@action("content")
@actionSeparator("/")
@doc("Get the content of the file.")
@get
getContent is SessionResourceFilesOperations.ResourceAction<
SessionResourceFile,
{},
{
@body _: bytes;
}
>;
Please also remove now unused model SessionResourceFileNamePathParameter from models/common.tsp.
There was a problem hiding this comment.
I will remove -1 on the review as I will be OOF tomorrow. But please do fix this.
There was a problem hiding this comment.
Thank you, this is fixed now
|
|
||
| #suppress "@azure-tools/typespec-azure-core/no-unknown" "" | ||
| @doc("The result of the code execution. The type of this field is same as the type of actual result of the code execution after being Json serialized.") | ||
| executionResult?: unknown; |
There was a problem hiding this comment.
If this is a JSON result, please use Record<unknown>
There was a problem hiding this comment.
Hi, had to revert this as this executionResult is not a Json object but rather a Json value
| ], | ||
| "security": [ | ||
| { | ||
| "AadOauth2Auth": [ |
There was a problem hiding this comment.
should it start from lower case?
There was a problem hiding this comment.
This is auto generated by TypeSpec, I searched other projects in this repo also found several similar patterns, I think it might be fine.
| } | ||
| ], | ||
| "responses": { | ||
| "202": { |
There was a problem hiding this comment.
only 202 is allowed to return? what about 200?
There was a problem hiding this comment.
This follows this guideline to only return 202 not 200 for LRO as a previous comment says
...app/data-plane/Microsoft.App.DynamicSessions/preview/2024-10-02-preview/DynamicSessions.json
Outdated
Show resolved
Hide resolved
...app/data-plane/Microsoft.App.DynamicSessions/preview/2024-10-02-preview/DynamicSessions.json
Outdated
Show resolved
Hide resolved
...app/data-plane/Microsoft.App.DynamicSessions/preview/2024-10-02-preview/DynamicSessions.json
Outdated
Show resolved
Hide resolved
...app/data-plane/Microsoft.App.DynamicSessions/preview/2024-10-02-preview/DynamicSessions.json
Outdated
Show resolved
Hide resolved
| } | ||
| }, | ||
| "post": { | ||
| "operationId": "SessionResourceFiles_UploadFile", |
There was a problem hiding this comment.
upload is not long running?
There was a problem hiding this comment.
Yes, currently only execute code can be long running
...app/data-plane/Microsoft.App.DynamicSessions/preview/2024-10-02-preview/DynamicSessions.json
Outdated
Show resolved
Hide resolved
...app/data-plane/Microsoft.App.DynamicSessions/preview/2024-10-02-preview/DynamicSessions.json
Outdated
Show resolved
Hide resolved
| } | ||
| }, | ||
| "delete": { | ||
| "operationId": "SessionResourceFiles_DeleteFile", |
There was a problem hiding this comment.
delete is not long running?
There was a problem hiding this comment.
Yes, currently only execute code can be long running
...app/data-plane/Microsoft.App.DynamicSessions/preview/2024-10-02-preview/DynamicSessions.json
Outdated
Show resolved
Hide resolved
| }, | ||
| "x-ms-identifiers": [] | ||
| }, | ||
| "innererror": { |
There was a problem hiding this comment.
| "innererror": { | |
| "innerError": { |
There was a problem hiding this comment.
This is also auto-generated by TypeSpec
| "type": "string", | ||
| "description": "One of a server-defined set of error codes." | ||
| }, | ||
| "innererror": { |
There was a problem hiding this comment.
| "innererror": { | |
| "innerError": { |
There was a problem hiding this comment.
This is also auto-generated by TypeSpec
| "items": { | ||
| "$ref": "#/definitions/SessionResourceFile" | ||
| }, | ||
| "x-ms-identifiers": [] |
There was a problem hiding this comment.
should name be identifiers?
yitaopan
added
the
PublishToCustomers
mikekistler
added
the
APIStewardshipBoard-SignedOff
Choose a PR Template
Switch to "Preview" on this description then select one of the choices below.
Click here to open a PR for a Data Plane API.
Click here to open a PR for a Control Plane (ARM) API.
fix #30393