Add [Resource]Container class by isra-fel · Pull Request #1180 · Azure/autorest.csharp

isra-fel · 2021-04-20T16:14:15Z

Description

Now the mgmt track 2 generator generates meaningful [Resource]Container classes.

This will be opened as draft for some early review. At the meantime I need to resolve some todo's and refactoring.
CI is expected to fail as this is pending Azure/azure-sdk-for-net#20524

Checklist

To ensure a quick review and merge, please ensure:

The PR has a understandable title and description explaining the why and what.
The PR is opened in draft if not ready for review yet.
- If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

Build is completely green
- Submissions with test failures require tracking issue and approval of a CODEOWNER
At least one +1 review by a CODEOWNER
All -1 reviews are confirmed resolved by the reviewer
- Override/Marking reviews stale must be discussed with CODEOWNERS first

…into container

isra-fel · 2021-04-25T07:54:01Z

/azp run autorest.csharp - ci

azure-pipelines · 2021-04-25T07:54:13Z

Azure Pipelines successfully started running 1 pipeline(s).

…into container

samples/Azure.ResourceManager.Sample/Generated/AvailabilitySetContainer.cs

src/AutoRest.CSharp/Mgmt/Generation/ResourceContainerWriter.cs

samples/Azure.ResourceManager.Sample/Generated/AvailabilitySetContainer.cs

…into container

samples/Azure.ResourceManager.Sample/Generated/AvailabilitySetContainer.cs

…into container

samples/Azure.Management.Storage/Generated/BlobContainer.cs

m-nash · 2021-05-10T19:36:58Z

samples/Azure.Management.Storage/Generated/BlobContainerContainer.cs

+        private BlobContainersRestOperations _restClient => new BlobContainersRestOperations(_clientDiagnostics, _pipeline, Id.SubscriptionId);
+
+        /// <summary> Typed Resource Identifier for the container. </summary>
+        public new ResourceGroupResourceIdentifier Id => base.Id as ResourceGroupResourceIdentifier;


This new shouldn't be needed

Without the new, Id is just a ResourceIdentifier inherited from OperationBase. We need to cast it either here or at the level of ContainerBase

~~A PR to arm.core~~
Though the original plan was to do the type conversion in OpeationsBase but problem is, TIdentifier has different meaning in Operation and Container.
Michael said he'll consider that later

samples/Azure.Management.Storage/Generated/BlobContainerContainer.cs

m-nash · 2021-05-10T19:40:08Z

samples/Azure.Management.Storage/Generated/BlobContainerOperations.cs

-        /// <summary> Initializes a new instance of <see cref = "BlobContainerOperations"/> class. </summary>
+        /// <summary> Initializes a new instance of the <see cref="BlobContainerOperations"/> class. </summary>
+        /// <param name="genericOperations"> An instance of <see cref="GenericResourceOperations"/> that has an id for a {todo: availability set}. </param>
+        internal BlobContainerOperations(GenericResourceOperations genericOperations) : base(genericOperations, genericOperations.Id)


I don't think this constructor is needed

When user lists resource with name filter, we list them as generic resource and create resource instances from the generic resources.

src/AutoRest.CSharp/Common/Output/Models/Types/ObjectType.cs

src/AutoRest.CSharp/Mgmt/AutoRest/MgmtTarget.cs

src/AutoRest.CSharp/Mgmt/Decorator/InheritanceChoser.cs

src/AutoRest.CSharp/Mgmt/Generation/ResourceContainerWriter.cs

src/AutoRest.CSharp/Mgmt/Generation/MgmtPlaneLongRunningOperationWriter.cs

src/AutoRest.CSharp/Mgmt/Generation/ResourceIdentifierChooser.cs

…into container

isra-fel · 2021-05-14T17:04:58Z

src/AutoRest.CSharp/Common/Output/Models/Requests/RestClientMethod.cs

    internal class RestClientMethod
    {
-        public RestClientMethod(string name, string? description, CSharpType? returnType, Request request, Parameter[] parameters, Response[] responses, DataPlaneResponseHeaderGroupType? headerModel, bool bufferResponse, string accessibility)
+        public RestClientMethod(string name, string? description, CSharpType? returnType, Request request, Parameter[] parameters, Response[] responses, DataPlaneResponseHeaderGroupType? headerModel, bool bufferResponse, string accessibility, Operation? operation = null)


In order to get the Operation object in container writer, I keep a reference to it in every RestClientMethod.
This is safe because every RestClientMethod is built from an Operation.
My only concern is if I should touch LowLevelClientMethod which inherits RestClientMethod to also add Operation to its constructor. see https://github.com/Azure/autorest.csharp/blob/feature/v3/src/AutoRest.CSharp/LowLevel/Output/LowLevelClientMethod.cs#L11

m-nash · 2021-05-14T20:11:45Z

samples/Azure.Management.Storage/Generated/BlobContainerContainer.cs

+        /// <param name="containerName"> The name of the blob container within the specified storage account. Blob container names must be between 3 and 63 characters in length and use numbers, lower-case letters and dash (-) only. Every dash (-) character must be immediately preceded and followed by a letter or number. </param>
+        /// <param name="blobContainer"> Properties of the blob container to create. </param>
+        /// <param name="cancellationToken"> A token to allow the caller to cancel the call to the service. The default value is <see cref="P:System.Threading.CancellationToken.None" />. </param>
+        public Operation<BlobContainer> StartCreateOrUpdate(string containerName, BlobContainerData blobContainer, CancellationToken cancellationToken = default)


need to change the reaponse type to be the LRO object

samples/Azure.Management.Storage/Generated/BlobContainerContainer.cs

m-nash · 2021-05-14T20:12:37Z

samples/Azure.Management.Storage/Generated/BlobContainerContainer.cs

+                    throw new ArgumentNullException(nameof(blobContainer));
+                }
+
+                return StartCreateOrUpdate(containerName, blobContainer, cancellationToken: cancellationToken).WaitForCompletion() as Response<BlobContainer>;


after response type change call waitforcompletion here

samples/Azure.Management.Storage/Generated/BlobServiceContainer.cs

src/AutoRest.CSharp/Mgmt/Decorator/InheritanceChooser.cs

src/AutoRest.CSharp/Mgmt/Generation/MgmtPlaneLongRunningOperationWriter.cs

ShivangiReja · 2021-05-14T21:32:56Z

src/AutoRest.CSharp/Mgmt/Generation/ResourceContainerWriter.cs

+                else if (IsStringLike(parameter.Type) && IsMandatory(parameter))
+                {
+                    passThru = false;
+                    if (string.Equals(parameter.Name, "resourceGroupName", StringComparison.InvariantCultureIgnoreCase))


Why are only checking resourceGroupName here? It could be subscriptionId, location ..etc

ShivangiReja · 2021-05-14T21:37:06Z

src/AutoRest.CSharp/Mgmt/Generation/ResourceOperationWriter.cs

-            writer.WriteXmlDocumentationSummary($"Initializes a new instance of {resourceOperation.Type.Name} for mocking.");
-            using (writer.Scope($"protected {resourceOperation.Type.Name}()"))
-            {
+            var typeOfThis = resourceOperation.Type.Name;


We shouldn't use the name here. We should use the Type.

test/TestProjects/ExactMatchInheritance/ExactMatchInheritance.json

src/AutoRest.CSharp/Mgmt/Generation/ResourceContainerWriter.cs

- remove outdated LRO writer - move `IsResource` to `ResourceData` - find Get rest method by prefix ("Get*") instead of just full match - this made `BlobServiceContainer` inheirt `ResourceContainerBase` corrrectly - and use the method name instead of hard code "Get" - WaitForCompletionAsync - empty lines at file end

container

bc38c37

isra-fel requested review from ShivangiReja, chamons and pakrym as code owners April 20, 2021 16:14

isra-fel marked this pull request as draft April 20, 2021 16:14

isra-fel added the Mgmt This issue is related to a management-plane library. label Apr 20, 2021

isra-fel and others added 12 commits April 21, 2021 16:52

stateful writer

89abbcb

new XXXCotnainer(ResourceOperationsBase)

2c0843c

refactor naming

679bb3e

refactor + resolve todos

59803a0

official arm.core build; stateful container writer

4d70df9

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

99085a3

…into container

hard code diag scope

512324a

dig scope cont.

44a5336

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

d7480e2

…into container

regen storage

8f854d5

storage can build

af3b106

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

9d9a7b1

…into container

Yeming Liu and others added 10 commits April 26, 2021 16:07

RESOLVE build errors in ExactMatchInheritance

2c678b3

fix MgmtParent

d10dae1

run generate.ps1

8573d48

add back default protected ctor

51d30ef

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

c11a094

…into container

clean up code

3282f3e

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

72dc6c2

…into container

regen w/ latest core; container.createXXX not override

0ed8f29

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

4b85fee

…into container

fix CI

fa707a7

do not write empty createXXX when there is no PUT

1ec22d3

isra-fel commented Apr 30, 2021

View reviewed changes

samples/Azure.ResourceManager.Sample/Generated/AvailabilitySetContainer.cs Outdated Show resolved Hide resolved

isra-fel commented Apr 30, 2021

View reviewed changes

Yeming Liu and others added 3 commits May 6, 2021 09:38

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

ffc830c

…into container

review comments

5cd76a0

no PhArmResponse

133fe35

isra-fel commented May 7, 2021

View reviewed changes

samples/Azure.ResourceManager.Sample/Generated/AvailabilitySetContainer.cs Outdated Show resolved Hide resolved

isra-fel force-pushed the container branch 2 times, most recently from d56675b to 26e0fc7 Compare May 10, 2021 10:00

paging; storage

25b8313

isra-fel force-pushed the container branch from 26e0fc7 to 25b8313 Compare May 10, 2021 14:30

Yeming Liu added 2 commits May 10, 2021 22:54

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

15fca6d

…into container

listAsync paging

e9d86e1

m-nash requested changes May 10, 2021

View reviewed changes

isra-fel and others added 7 commits May 11, 2021 15:48

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

840301f

…into container

review comments

7f1cebd

ArmResponse -> Response

2073e8c

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

200d516

…into container

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

8c4030e

…into container

integrate LRO code

3bafe21

split list to overrides

efb6eec

isra-fel commented May 14, 2021

View reviewed changes

isra-fel marked this pull request as ready for review May 14, 2021 17:05

isra-fel requested a review from allenjzhang as a code owner May 14, 2021 17:05

m-nash approved these changes May 14, 2021

View reviewed changes

ShivangiReja reviewed May 14, 2021

View reviewed changes

isra-fel mentioned this pull request May 17, 2021

Convert [Resource]Data to [Resource] in LRO #1244

Merged

6 tasks

m-nash merged commit d3ffd61 into Azure:feature/v3 May 17, 2021

Comments

Conversation

isra-fel commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Ready to Land?

Uh oh!

isra-fel commented Apr 25, 2021

Uh oh!

azure-pipelines bot commented Apr 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isra-fel May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isra-fel May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

isra-fel commented Apr 20, 2021 •

edited

Loading

isra-fel May 12, 2021 •

edited

Loading

isra-fel May 11, 2021 •

edited

Loading