Implement mgmt resource #47944

live1206 · 2025-01-23T00:10:43Z

Resolves Implement ResourceProvdier #47744
Resolves Generate OperationSource in Azure plugin #48135
Add test to verify the generation
Generated MgmtTypeSpec test can build now

eng/packages/http-client-csharp/generator/Azure.Generator/test/TestHelpers/Configuration.json

m-nash

Add it to the Local.Tests.Csproj like the dpg here https://github.com/Azure/azure-sdk-for-net/blob/main/eng/packages/http-client-csharp/generator/TestProjects/Local.Tests/TestProjects.Local.Tests.csproj#L22-L23

m-nash · 2025-02-14T17:19:34Z

eng/scripts/typespec/Generate-Code.ps1

-# temporarily disable building MgmtTypeSpec because now the generated code of this project cannot build
-# Write-Host "Building BasicTypeSpec" -ForegroundColor Cyan
-# Invoke-LoggedCommand "dotnet build $packageRoot/generator/TestProjects/Local/Mgmt-TypeSpec/src/MgmtTypeSpec.csproj"
+Write-Host "Building BasicTypeSpec" -ForegroundColor Cyan


Why do we have a Generate-Code.ps1 here? Shouldn't we be using /eng/packages/http-client-csharp/eng/scripts/Generate.ps1 which follows the pattern of ms/tsp as well?

/eng/packages/*/eng/scripts was moved to /eng/scripts in #46511 by @hallipr.
Since we have more logic in /eng/packages/http-client-csharp/eng/scripts/Generate.ps1 now, I will add Mgmt-TypeSpec generation to it in this PR.
Then, we can sync with @hallipr if it should be moved to /eng/scripts

live1206 · 2025-02-18T02:35:51Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/ResourceProvider.cs

+        {
+            // TODO: use _clientProvider.CanonicalView instead when it implements BuildMethods
+            MethodProvider[] methods = [.. _clientProvider.Methods, .. _clientProvider.CustomCodeView?.Methods ?? []];
+            return methods.Single(m => m.Signature.Name.Equals(isAsync ? $"{operation.Name}Async" : operation.Name, StringComparison.OrdinalIgnoreCase) && m.Signature.Parameters.Any(p => p.Type.Equals(typeof(CancellationToken))));


It should be operation.Name.ToCleanName() instead of operation.Name. We need to expose StringExtension.ToCleanName

So that, we can get the corresponding RestClientProvider while generating Resource in Azure plugin. Corresponding PR in Azure plugin: Azure/azure-sdk-for-net#47944

Copilot

Copilot reviewed 30 out of 45 changed files in this pull request and generated 2 comments.

Files not reviewed (15)

eng/Packages.Data.props: Language not supported
eng/packages/http-client-csharp/eng/scripts/Generate.ps1: Language not supported
eng/packages/http-client-csharp/generator/Azure.Generator/src/Properties/launchSettings.json: Language not supported
eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/MgmtLongRunningOperationProvider.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/ResourceDataSerializationProvider.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/Utilities/OperationExtensions.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/RestClientVisitor.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/Mgmt/Models/OperationSet.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/ResourceDataProvider.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/Abstraction/HttpPipelineProvider.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/AzureClientPlugin.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/AzureOutputLibrary.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/ResourceProvider.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/Utilities/ResourceDetection.cs: Evaluated as low risk
eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/OperationSourceProvider.cs: Evaluated as low risk

Comments suppressed due to low confidence (2)

eng/packages/http-client-csharp/generator/Azure.Generator/src/Utilities/MethodExtensions.cs:1

The copyright symbol is not correctly displayed. It should be corrected to '©'.

// Copyright (c) Microsoft Corporation. All rights reserved.

eng/packages/http-client-csharp/generator/Azure.Generator/src/Mgmt/Models/RequestPath.cs:101

The diff should be constructed using a delimiter. Change to: diff = new RequestPath(string.Join("/", other._segments.Skip(Count)));

diff = new RequestPath(string.Join("", other._segments.Skip(Count)));

eng/packages/http-client-csharp/generator/Azure.Generator/src/Utilities/MethodExtensions.cs

eng/packages/http-client-csharp/generator/Azure.Generator/src/Mgmt/Models/RequestPath.cs

…Mgmt/Models/RequestPath.cs Co-authored-by: Copilot <[email protected]>

m-nash · 2025-02-24T17:52:54Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/ResourceProvider.cs

+
+namespace Azure.Generator.Providers
+{
+    internal class ResourceProvider : TypeProvider


Lets call this ResourceClientProvider and see if we can inherit from ClientProvider.

As offline discussed, we would like to reorg the rest clients, operation clients and resource clients. So that, the structure of mgmt resource clients are similar to the DPG clients.

m-nash · 2025-02-24T17:54:18Z

...tp-client-csharp/generator/Azure.Generator/src/Providers/MgmtLongRunningOperationProvider.cs

@@ -174,7 +174,7 @@ private ConstructorProvider BuildInitializationConstructor()
                    skipApiVersionOverrideParameter,
                    apiVersionOverrideValueParameter,
                };
-            var signature = new ConstructorSignature(Type, $"", MethodSignatureModifiers.Internal, parameters, null);
+            var signature = new ConstructorSignature(Type, $"", MethodSignatureModifiers.Internal, parameters);


Is there any reason this class is prefixed with Mgmt? I think Dpg and Mpg LROs work the same other than the type that gets used ArmOperation vs Operation?

For now, it only contains logic for Mgmt, rename it to LongRunningOperationProvider.

eng/packages/http-client-csharp/eng/scripts/Generate.ps1

m-nash · 2025-02-24T19:47:34Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/AzureOutputLibrary.cs

-        private Dictionary<string, OperationSet> _pathToOperationSetMap;
-        private Dictionary<string, HashSet<OperationSet>> _resourceDataBySpecNameMap;
+        //TODO: Move these to InputLibrary instead
+        private Dictionary<RequestPath, OperationSet> _pathToOperationSetMap;


There is a lot of code in here that is mgmt specific and it starts to get difficult to tell which is which have we considered a mgmt plugin?

Most likely.
Given I am fully working with MPG now, I think it would be better to split them when we have more implementation on DPG, then we can clarify the things in common for both.

m-nash · 2025-02-24T19:49:22Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/AzureClientPlugin.cs

@@ -30,6 +31,9 @@ public class AzureClientPlugin : ScmCodeModelPlugin
    public override AzureOutputLibrary OutputLibrary => _azureOutputLibrary ??= new();

    internal ResourceDetection ResourceDetection { get; } = new();
+    internal ParentDetection ParentDetection { get; } = new();


Isn't your parent just a property on the client from typespec?

For TypeSpec, we should be able to get the resource hierarchy directly via #48281.
The resource detection logic is mainly for swagger input.
For now, given getArmResources API is not ready for integration. We use it as a temporary workaround.

Eventually, when we have the native way ready, we should get rid of them. And the resource detection should be part of the M4 output conversion to tspCodeModel.json in autorest.csharp.
Added TODO to remove them.

As offline discussed, we need to clarify the contract of input types containing the resource hierarchy. And it should be an extensible way of holding it in Azure plugin only.
Then, we can put the temp workaround of resource detection close to the input instead of output.

...ckages/http-client-csharp/generator/Azure.Generator/src/Providers/OperationSourceProvider.cs

eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/ResourceProvider.cs

m-nash · 2025-02-24T19:56:05Z

...s/http-client-csharp/generator/TestProjects/Local/Mgmt-TypeSpec/src/Generated/FooResource.cs

+            TryGetApiVersion(ResourceType, out string fooApiVersion);
+            _fooRestClient = new Foos(Pipeline, Endpoint, fooApiVersion);
+#if DEBUG
+            global::MgmtTypeSpec.FooResource.ValidateResourceId(id);


Is this not getting reduced because its in an if debug?

oh, you were talking about the namespace not reduced.
Yes. I tried to remove the debug block, the namespace was simplified.

raise issue in MGC to fix this
we can replace it with Debug.Assert here

Debug.Assert can't be used here, it needs a bool condition input, but this method is void.
Use [Conditional("DEBUG")] in the method signature instead.

microsoft/typespec#6160 to track the issue in MGC

m-nash · 2025-02-24T19:57:22Z

...Generator/test/Providers/TestData/ResourceProviderTests/Verify_ResourceProviderGeneration.cs

+namespace Samples
+{
+    /// <summary></summary>
+    public partial class ResponseTypeResource : global::Azure.ResourceManager.ArmResource


Lets trim this down to at most 1 method validation at a time. Otherwise this test will be susceptible to any change in generation of any portion of a resource

Is there a way to write a method of TypeProvider?
As I can see, there is no public API from TypeProviderWriter. Do we want to expose such API?

Separated the tests for ResourceClientProvider.
And after exposing AzureOutputLibrary.CreateResourceCore, I realized that we need to simplify the creation of ResourceClientProvider. And it ties to the contract of Input types from MGC.

...ackages/http-client-csharp/generator/Azure.Generator/test/Providers/ResourceProviderTests.cs

eng/Packages.Data.props

live1206 added 5 commits January 16, 2025 11:02

wip

f041f24

wip

badf1a5

wip

69fa28a

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

615fec1

wip

d136be2

github-actions bot added Client This issue points to a problem in the data-plane of the library. CodeGen Issues that relate to code generation labels Jan 23, 2025

live1206 added 7 commits January 26, 2025 09:21

wip

a040627

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

0296a71

generate operation methods for Resource

fda752a

add package back

580508a

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

ba12225

update to latest MGC

ef83882

fix test

120e164

live1206 mentioned this pull request Feb 6, 2025

Expose RestClient from ClientProvider microsoft/typespec#5888

Merged

live1206 added 3 commits February 7, 2025 13:41

Merge branch 'main' into implement-mgmt-resource

e4bbde0

Add test for ResourceProvider

b2842c4

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

5f5cee9

live1206 commented Feb 8, 2025

View reviewed changes

eng/packages/http-client-csharp/generator/Azure.Generator/test/TestHelpers/Configuration.json Outdated Show resolved Hide resolved

Add RequestPath

41cf53c

live1206 marked this pull request as ready for review February 8, 2025 07:42

live1206 requested review from JoshLove-msft, m-nash, jorgerangel-msft, jsquire, annelo-msft, christothes, KrzysztofCwalina, tg-msft and ArthurMa1978 as code owners February 8, 2025 07:42

m-nash reviewed Feb 14, 2025

View reviewed changes

live1206 added 2 commits February 15, 2025 16:20

Address comments

adbcc1c

Add Mgmt-TypeSpec generation to Generate.ps1

bb92673

live1206 commented Feb 18, 2025

View reviewed changes

live1206 mentioned this pull request Feb 18, 2025

Replace ResourceDataProvider with ResourceVisitor #48337

Closed

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

af9bd9c

live1206 force-pushed the implement-mgmt-resource branch from 0d47984 to af9bd9c Compare February 19, 2025 00:34

live1206 requested a review from Copilot February 19, 2025 01:02

Copilot AI reviewed Feb 19, 2025

View reviewed changes

eng/packages/http-client-csharp/generator/Azure.Generator/src/Utilities/MethodExtensions.cs Show resolved Hide resolved

eng/packages/http-client-csharp/generator/Azure.Generator/src/Mgmt/Models/RequestPath.cs Outdated Show resolved Hide resolved

live1206 and others added 8 commits February 19, 2025 09:27

Update eng/packages/http-client-csharp/generator/Azure.Generator/src/…

c5ca437

…Mgmt/Models/RequestPath.cs Co-authored-by: Copilot <[email protected]>

update

3ed339d

Merge branch 'main' into implement-mgmt-resource

d800d35

fix build

8d78f7f

update to use CanonicalView

8fbc8aa

update test

081696d

Use ModelSerializationExtensions.WireOptions instead of new instance

f9e8afd

update test

8d7b831

m-nash reviewed Feb 24, 2025

View reviewed changes

jsquire reviewed Feb 24, 2025

View reviewed changes

eng/Packages.Data.props Outdated Show resolved Hide resolved

live1206 added 10 commits February 25, 2025 11:57

Address comments

4dcccc7

typo

d00ee3a

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

0086b11

resolve comments

e34db7b

fix build

417af63

update test

965e951

separate tests for ResourceClientProvider

579dd9b

Add TODO

682e704

fix tests

22e08c6

Move resource related logic to ResourceBuilder

541193c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement mgmt resource #47944

Implement mgmt resource #47944

live1206 commented Jan 23, 2025 •

edited

Loading

m-nash left a comment

m-nash Feb 14, 2025

live1206 Feb 15, 2025 •

edited

Loading

live1206 Feb 18, 2025

Copilot AI left a comment

m-nash Feb 24, 2025

live1206 Feb 26, 2025

m-nash Feb 24, 2025

live1206 Feb 25, 2025

m-nash Feb 24, 2025

live1206 Feb 25, 2025 •

edited

Loading

m-nash Feb 24, 2025

live1206 Feb 25, 2025 •

edited

Loading

live1206 Feb 26, 2025

m-nash Feb 24, 2025

live1206 Feb 25, 2025 •

edited

Loading

live1206 Feb 26, 2025 •

edited

Loading

live1206 Feb 26, 2025

live1206 Feb 26, 2025

m-nash Feb 24, 2025

live1206 Feb 25, 2025

live1206 Feb 27, 2025

Implement mgmt resource #47944

Are you sure you want to change the base?

Implement mgmt resource #47944

Conversation

live1206 commented Jan 23, 2025 • edited Loading

m-nash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

live1206 Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

live1206 Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

live1206 Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

live1206 Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

live1206 Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

live1206 commented Jan 23, 2025 •

edited

Loading

live1206 Feb 15, 2025 •

edited

Loading

live1206 Feb 25, 2025 •

edited

Loading

live1206 Feb 25, 2025 •

edited

Loading

live1206 Feb 25, 2025 •

edited

Loading

live1206 Feb 26, 2025 •

edited

Loading