[AutoPR Azure.Containers.Apps.Sandbox]-generated-from-SDK Generation - .NET-6871096 - #63296
azure-sdk-automation[bot] wants to merge 9 commits into
Conversation
….yaml', API Version: 2026-09-01-preview, SDK Release Type: beta, and CommitSHA: '7a7239408dbf572b10df6da25b72d17b6067aa2f' in SpecRepo: 'https://github.com/Azure/azure-rest-api-specs' Pipeline run: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=6871096 Refer to https://eng.ms/docs/products/azure-developer-experience/develop/sdk-release/sdk-release-prerequisites to prepare for SDK release.
azure-sdk-automation
Bot
added
the
auto-release
|
Azure Pipelines: Successfully started running 1 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The package lacks CI and tests, has incomplete release documentation, and exposes public naming issues that should be corrected before its first beta.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (6)
What changed in this PR
Introduces the initial beta Azure Container Apps Sandbox SDK generated from the 2026-09-01-preview TypeSpec specification.
Changes:
- Adds generated clients, operations, models, serialization, and paging support.
- Adds package metadata, API listings, and build configuration.
- Adds initial README and changelog scaffolding.
| File | Description |
|---|---|
tsp-location.yaml |
Pins the source specification and emitter. |
src/Azure.Containers.Apps.Sandbox.csproj |
Defines the new SDK package. |
src/Generated/ContainerAppsSandboxClient*.cs |
Implements client construction and configuration. |
src/Generated/SandboxGroup*.cs |
Implements sandbox-group operations. |
src/Generated/CollectionResults/*.cs |
Implements pageable operation results. |
src/Generated/Models/*.cs |
Defines and serializes service models. |
src/Generated/Internal/*.cs |
Provides generated request and serialization helpers. |
src/Generated/schema/ConfigurationSchema.json |
Defines configuration binding schema. |
api/*.cs |
Records the public API surface by target framework. |
README.md |
Adds package documentation scaffold. |
CHANGELOG.md |
Adds initial release-history scaffold. |
metadata.json |
Records the preview API version. |
Directory.Build.props |
Imports repository build settings. |
Azure.Containers.Apps.Sandbox.slnx |
Adds the package solution. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| @@ -0,0 +1,26 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| /// <param name="resourceGroupName"></param> | ||
| /// <param name="sandboxGroupName"></param> | ||
| /// <exception cref="ArgumentNullException"> <paramref name="subscriptionId"/>, <paramref name="resourceGroupName"/> or <paramref name="sandboxGroupName"/> is null. </exception> | ||
| public virtual SandboxGroup GetSandboxGroupClient(string subscriptionId, string resourceGroupName, string sandboxGroupName) |
| namespace Azure.Containers.Apps.Sandbox | ||
| { | ||
| /// <summary> Metadata for a file or directory in a sandbox. </summary> | ||
| public partial class FileInfo |
There was a problem hiding this comment.
I agree that FileInfo should become SandboxFileInfo to avoid the System.IO.FileInfo collision. Can we also expand the related abbreviations: MkDirContent to SandboxDirectoryContent, DirListingResult to SandboxDirectoryListingResult, and FileOpStatusResult to SandboxFileOperationResult?
For the metadata properties, prefer IsDirectory, IsSymbolicLink, and SymbolicLinkTarget over IsDir, IsSymlink, and SymlinkTarget. Keep the wire names unchanged.
🤖 Generated by Jorge's Copilot
There was a problem hiding this comment.
Ahmed ElSayed (@ahmelsayed) These suggestions overall is good to me, how do you think of it?
| ## Key concepts | ||
|
|
||
| ## Examples | ||
|
|
||
| ## Troubleshooting | ||
|
|
||
| ## Next steps |
| { | ||
| /// <summary> | ||
| /// Customer-supplied selector that picks one of the managed identities already on a sandbox group. | ||
| /// Please note this is the abstract base class. The derived classes available for instantiation are: <see cref="SandboxGroupIdentitySelectorSystemAssignedIdentitySelector"/> and <see cref="SandboxGroupIdentitySelectorUserAssignedIdentitySelector"/>. |
There was a problem hiding this comment.
Can we shorten the two subtype names to SystemAssignedSandboxGroupIdentitySelector and UserAssignedSandboxGroupIdentitySelector? They retain the base-type relationship without repeating “IdentitySelector.”
The existing abstract base and strongly typed authentication hierarchy can remain unchanged.
🤖 Generated by Jorge's Copilot
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| "dapr", | ||
| "grpc", | ||
| "proto" | ||
| "proto", |
There was a problem hiding this comment.
Please do not make changes to the root .cspell. Add a local entry.
There was a problem hiding this comment.
Richard chen (@RichardChen820) can we address this feedback please ?
There was a problem hiding this comment.
It seems be overwritten by re-generating, updated.
| /// <exception cref="ArgumentNullException"> <paramref name="path"/> is null. </exception> | ||
| /// <exception cref="ArgumentException"> <paramref name="path"/> is an empty string, and was expected to be non-empty. </exception> | ||
| /// <exception cref="RequestFailedException"> Service returned a non-success status code. </exception> | ||
| public virtual Response<BinaryData> DownloadSandboxFile(string path, string containerName = default, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
DownloadSandboxFile and DownloadVolumeFile currently return Response<BinaryData> and materialize the response content. Can we add custom convenience APIs that return Response<Stream> / Task<Response<Stream>>, or a download result exposing a Stream, for large files?
The implementation should disable response buffering before sending the request and keep the response stream alive for the caller to dispose. Returning BinaryData.ToStream() would not avoid buffering. Please provide sync/async variants with cancellation and explicit stream ownership, consistent with the large-payload guidance.
🤖 Generated by Jorge's Copilot
There was a problem hiding this comment.
This is indeed necessary, added
| /// <exception cref="ArgumentNullException"> <paramref name="path"/> or <paramref name="content"/> is null. </exception> | ||
| /// <exception cref="ArgumentException"> <paramref name="path"/> is an empty string, and was expected to be non-empty. </exception> | ||
| /// <exception cref="RequestFailedException"> Service returned a non-success status code. </exception> | ||
| public virtual Response<WriteFileResult> UploadSandboxFile(string path, BinaryData content, bool? createDirs = default, int? mode = default, string containerName = default, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Can we add custom Stream overloads for UploadSandboxFile, UploadVolumeFile, and UploadContentPackage so callers can upload file content without first materializing BinaryData?
These currently send raw binary bodies, not multipart/form-data. We can use RequestContent.Create(stream), or adapt FileBinaryContent through RequestContent.Create, without buffering the entire file. Please preserve the existing content types and define stream ownership and retry/replay behavior.
If the service actually supports multipart/form-data, model its content type and parts in TypeSpec and use the generator's multipart support rather than hand-writing that encoding. Multipart helpers would be useful in that case, but changing the content type alone would not preserve the current raw-body contract.
🤖 Generated by Jorge's Copilot
| namespace Azure.Containers.Apps.Sandbox | ||
| { | ||
| /// <summary> Pod volumes and container mounts to add to a sandbox. </summary> | ||
| public partial class AddPodVolumeMountsContent |
There was a problem hiding this comment.
Can we rename AddPodVolumeMountsContent to PodVolumeMountsContent? The method already expresses the action, while the model represents the pod volumes and container mounts supplied as content.
For the same naming family, consider SandboxVolumeMountContent instead of AddVolumeMountContent. These should be C#-only names, preserving the existing payload and operation behavior.
🤖 Generated by Jorge's Copilot
There was a problem hiding this comment.
Updated in spec client.tsp
| /// The source used to create a disk image. | ||
| /// Please note this is the abstract base class. The derived classes available for instantiation are: <see cref="CreateDiskImageSourceBlobSource"/> and <see cref="CreateDiskImageSourceRegistrySource"/>. | ||
| /// </summary> | ||
| public abstract partial class CreateDiskImageSource |
There was a problem hiding this comment.
The source-type names repeat the operation and hierarchy: CreateDiskImageSourceBlobSource and CreateDiskImageSourceRegistrySource. Could we use DiskImageSource as the base, with BlobDiskImageSource and RegistryDiskImageSource?
DiskImageImage would also be clearer as DiskImageContainerConfiguration, with BaseImageReference instead of Base. This describes its resolved image reference, entrypoint, and command without repeating “Image.”
🤖 Generated by Jorge's Copilot
There was a problem hiding this comment.
The source-type names repeat the operation and hierarchy: CreateDiskImageSourceBlobSource and CreateDiskImageSourceRegistrySource. Could we use DiskImageSource as the base, with BlobDiskImageSource and RegistryDiskImageSource?
This does make sense updated in spec client.tsp.
There was a problem hiding this comment.
DiskImageImage would also be clearer as DiskImageContainerConfiguration, with BaseImageReference instead of Base. This describes its resolved image reference, entrypoint, and command without repeating “Image.”
How about update it to ImageMetadata
There was a problem hiding this comment.
DiskImageImage would also be clearer as DiskImageContainerConfiguration, with BaseImageReference instead of Base. This describes its resolved image reference, entrypoint, and command without repeating “Image.”
How about update it to
ImageMetadata
that works too
| /// Defines a single column in a custom log output schema. Polymorphic on `kind` discriminator: `"Value"` or `"Ref"`. | ||
| /// Please note this is the abstract base class. The derived classes available for instantiation are: <see cref="RefLogColumnDef"/> and <see cref="ValueLogColumnDef"/>. | ||
| /// </summary> | ||
| public abstract partial class LogColumnDef |
There was a problem hiding this comment.
Can we expand LogColumnDef to TelemetryLogColumn, with ReferenceTelemetryLogColumn and LiteralTelemetryLogColumn for its derived types?
The authentication family would also be more consistent with BlobVolumeAuthentication if TelemetryAuth and the corresponding ...Auth model names used Authentication. TelemetryConfig could similarly become TelemetryConfiguration. These are naming refinements, not changes to the discriminators or JSON fields.
🤖 Generated by Jorge's Copilot
| } | ||
|
|
||
| /// <summary> The number of bytes received. </summary> | ||
| public long? RxBytes { get; } |
There was a problem hiding this comment.
Can we expand RxBytes / TxBytes to BytesReceived / BytesSent, and the corresponding packet properties to PacketsReceived / PacketsSent? Likewise, LoadAverage1Minute, LoadAverage5Minutes, and LoadAverage15Minutes are clearer than LoadAvg1, LoadAvg5, and LoadAvg15.
For command results, prefer StandardOutput and StandardError over Stdout and Stderr. For CPU counters such as Iowait, Irq, and Softirq, please confirm the time unit before choosing final names or TimeSpan mappings; we should not imply .NET ticks or percentages without that contract.
🤖 Generated by Jorge's Copilot
| public string Type { get; } | ||
|
|
||
| /// <summary> Current connection state. </summary> | ||
| public string State { get; } |
There was a problem hiding this comment.
Can we confirm the supported values for SandboxConnection.State and DiskImageStatus.State? If these are service-defined state sets, they should have appropriate extensible-enum types rather than undocumented strings.
Please only reuse the existing ConnectionState if it represents the same contract. Provider-specific connection types and opaque identifiers should remain strings where their values are genuinely open-ended.
🤖 Generated by Jorge's Copilot
There was a problem hiding this comment.
They are string type in server side.
Ahmed ElSayed (@ahmelsayed) What's the possible value of them?
There was a problem hiding this comment.
We have the extensible enum type https://azure.github.io/azure-sdk/dotnet_implementation.html#dotnet-enums which supports strings. The idea here is if there's a known list of possible values, the enum here can help constructing those. Any unknown values still get materialized as strings over + from the wire
There was a problem hiding this comment.
All updated to ResourceState
| namespace Azure.Containers.Apps.Sandbox | ||
| { | ||
| /// <summary> Customer-facing sandbox resource returned by the service. </summary> | ||
| public partial class ContainerAppsSandbox |
There was a problem hiding this comment.
ContainerAppsSandbox is currently the response-data model, while SandboxGroupSandbox is the object that performs operations. Can we name the data model ContainerAppsSandboxProperties to make that distinction explicit, following patterns such as ContainerRepository / ContainerRepositoryProperties?
This does not require changing the current hierarchy or factory methods.
🤖 Generated by Jorge's Copilot
There was a problem hiding this comment.
Doesn't have very strong opinion on this.
Ahmed ElSayed (@ahmelsayed) How do you think?
| /// <summary> The HTTP pipeline for sending and receiving REST requests and responses. </summary> | ||
| public virtual HttpPipeline Pipeline { get; } | ||
|
|
||
| /// <summary> The ClientDiagnostics is used to provide tracing support for the client library. </summary> |
There was a problem hiding this comment.
Can we add custom partial-class code to expose the captured identifiers as read-only virtual properties? For example, SubscriptionId, ResourceGroupName, and Name on SandboxGroup, and Id on the sandbox resource object. This needs to be implemented through custom code, not by editing generated files.
For the group's full ARM resource ID, we can optionally expose an Azure.Core.ResourceIdentifier property, following the earlier guidance. Keep the individual sandbox's opaque ID as a string. This follows the resource-subclient guidance.
🤖 Generated by Jorge's Copilot
| @@ -0,0 +1,4150 @@ | |||
| namespace Azure.Containers.Apps.Sandbox | |||
There was a problem hiding this comment.
The package exposes roughly 200 model-related types in the same root namespace as the clients. Can we use the C# emitter's model-namespace option to place models in Azure.Containers.Apps.Sandbox.Models?
In tspconfig.yaml, set:
options:
"@azure-typespec/http-client-csharp":
namespace: Azure.Containers.Apps.Sandbox
model-namespace: trueThen regenerate and align any custom partial classes with the resulting namespaces. The .NET guidelines allow this for model-heavy libraries; it is an organizational refinement best settled before publication.
🤖 Generated by Jorge's Copilot
| public SandboxEgressPolicy EgressPolicy { get; } | ||
|
|
||
| /// <summary> Optional sandbox group identifier. </summary> | ||
| public string SandboxGroupId { get; } |
There was a problem hiding this comment.
The earlier review confirmed that ContainerAppsSandbox.SandboxGroupId and CreateSandboxGatewayConnectionContent.ResourceId are full ARM resource IDs, not GUIDs, and confirmed that data-plane libraries can use Azure.Core.ResourceIdentifier.
Can we consider applying that typing through custom code to these model properties and EgressPolicyManagedIdentityRef.IdentityResourceId, preserving their JSON string representation and optionality? This is separate from exposing identity properties on subclients. Opaque sandbox and connection IDs should remain strings.
🤖 Generated by Jorge's Copilot
| public string Name { get; } | ||
|
|
||
| /// <summary> Connection type. </summary> | ||
| public string Type { get; } |
There was a problem hiding this comment.
CreateConnectionContent.Type and SandboxConnection.Type still use string. Are these values selected from a service-defined set, or are arbitrary provider-defined values supported?
If there are documented well-known values, can we use a shared extensible-enum type for the request and response while retaining support for unknown values? This is the connection type, separate from the connection state discussed in the other comment.
🤖 Generated by Jorge's Copilot
There was a problem hiding this comment.
They are all string in backend service
There was a problem hiding this comment.
We might want to consider a string extensible enum https://azure.github.io/azure-sdk/dotnet_implementation.html#dotnet-enums if there's a known value set.
There was a problem hiding this comment.
The type field in this request is a connector ID, not a fixed enum value. If the specified connector type is not available in the target region, the service returns ConnectionTypeNotAvailable.
The existing SDK also models this field as a string rather than an enum:
https://msft.ghe.com/coreai/adc/blob/858d9a9a2294a6bac6b3016813ddb99c4731e4b0/client-sdk/csharp/csharp-core/src/Microsoft.Adc.Client.Core/Models/Connection.cs#L17
* Add live and unit tests * Add asserts * add CI pipeline
| public override async IAsyncEnumerable<Page<TResource>> AsPages(string continuationToken = default, int? pageSizeHint = default) | ||
| { | ||
| await foreach (Page<TModel> page in _source.AsPages(continuationToken, pageSizeHint).ConfigureAwait(false)) |
There was a problem hiding this comment.
Not required for beta:
Can we make the async list APIs honor cancellation supplied during enumeration, as well as cancellation passed to the service method?
For example, group.GetSandboxesAsync().WithCancellation(token) continues fetching pages after the token is cancelled. The same happens with .AsPages().WithCancellation(token). Passing the token directly to GetSandboxesAsync(cancellationToken: token) works, so these otherwise equivalent usage patterns currently behave differently.
Please propagate the enumeration token through the resource-page adapter and the underlying pageable requests, preserving any method-level token too. The generated pageable has the same gap, and I believe we are working on a fix.
🤖 Generated by Jorge's Copilot
There was a problem hiding this comment.
Looks like this is also a problem in our generated code #63508, so feel free to ignore this. Though it might be useful to fix in the hand written implementation at some point.
|
|
||
| /// <summary> Gets the current sandbox properties in a new resource client. This instance's <see cref="Data"/> remains unchanged. </summary> | ||
| /// <param name="cancellationToken"> The cancellation token that can be used to cancel the operation. </param> | ||
| public virtual Response<SandboxResource> Get(CancellationToken cancellationToken = default) | ||
| { | ||
| Response<SandboxProperties> response = _client.GetProperties(Id, cancellationToken); | ||
| return Response.FromValue(new SandboxResource(_client, Id, response.Value), response.GetRawResponse()); | ||
| } | ||
|
|
||
| /// <summary> Gets the current sandbox properties in a new resource client asynchronously. This instance's <see cref="Data"/> remains unchanged. </summary> | ||
| /// <param name="cancellationToken"> The cancellation token that can be used to cancel the operation. </param> | ||
| public virtual async Task<Response<SandboxResource>> GetAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| Response<SandboxProperties> response = await _client.GetPropertiesAsync(Id, cancellationToken).ConfigureAwait(false); | ||
| return Response.FromValue(new SandboxResource(_client, Id, response.Value), response.GetRawResponse()); |
There was a problem hiding this comment.
Not required for beta:
Is the intended public experience the resource facade, the generated operation-group clients, or both? It would be good to settle that boundary before the initial release.
For example, callers can fetch volume state through group.GetVolumesClient().GetVolumeAsync(name), group.GetVolume(name).GetAsync(), or group.GetVolume(name).GetVolumeAsync(). The latter two are on the same resource facade: one returns a new resource with Data populated, while the other returns the model directly. Neither refreshes the original resource's Data.
If the facade is the primary API, would it make sense to keep the generated operation-group clients and their factories internal, and expose one retrieval convention on each resource? Either Get[Async] returning a resource/data snapshot or GetProperties[Async] returning the model can be a deliberate choice. Hiding the generated layer alone would still leave the two retrieval forms on the facade.
If both public layers and retrieval forms are intentional, what distinct customer scenarios should guide callers toward each? Whichever approach we choose, let's apply it consistently across the resource types and preserve access to needed operations, paging, and protocol/custom-request capabilities.
🤖 Generated by Jorge's Copilot
There was a problem hiding this comment.
Per https://eng.ms/docs/products/azure-developer-experience/develop/sdk-samples.html, can we add at least 1 sample for the common use case ? I see those were added directly into the README, but can we please use the sample snippet generator so those samples are included as part of the test runs ? Guides on how to do that:
| @@ -0,0 +1,23 @@ | |||
| # NOTE: Please refer to https://aka.ms/azsdk/engsys/ci-yaml before editing this file. | |||
There was a problem hiding this comment.
It looks like I missed that we may need to create the initial pipeline itself for live tests https://github.com/Azure/azure-sdk-for-net/blob/main/doc/DataPlaneCodeGeneration/Azure_SDK_Package_Ship_Requirements.md#test-pipelines. Can you please take a look ?
| @@ -0,0 +1,21 @@ | |||
| # Release History | |||
|
|
|||
| ## 1.0.0-beta.1 (Unreleased) | |||
There was a problem hiding this comment.
Once we have a release date in mind, we'll need to replace Unreleased with the release date in (YYYY-MM-DD) format. Since this PR is tagged with auto-release, I believe we need to do that as part of this pr.
Configurations: 'specification/app/data-plane/ContainerApps/tspconfig.yaml', API Version: 2026-09-01-preview, SDK Release Type: beta, and CommitSHA: '7a7239408dbf572b10df6da25b72d17b6067aa2f' in SpecRepo: 'https://github.com/Azure/azure-rest-api-specs' Pipeline run: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=6871096 Refer to https://eng.ms/docs/products/azure-developer-experience/develop/sdk-release/sdk-release-prerequisites to prepare for SDK release. Release plan link: https://azsdk-releaseplan-dashboard-hveph5aqhhcfhtgu.westus-01.azurewebsites.net/?releaseplan=36625 Submitted by: Junbo.Chen@microsoft.com