Service Fabric Managed Clusters - initial skd with api version 2021-01-01-preview #18875

a-santamaria · 2021-02-19T05:48:20Z

All SDK Contribution checklist:

Support for service fabric managed clusters with the api version 2020-01-01-preview swagger link: https://github.com/Azure/azure-rest-api-specs/tree/master/specification/servicefabricmanagedclusters/resource-manager/Microsoft.ServiceFabricManagedClusters/preview/2021-01-01-preview

swagger prs:
initial pr: Azure/azure-rest-api-specs#12639
missing properties and fixes: Azure/azure-rest-api-specs#12858

This checklist is used to make sure that common guidelines for a pull request are followed.

Please open PR in Draft mode if it is:
- Work in progress or not intended to be merged.
- Encountering multiple pipeline failures and working on fixes.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
I have read the contribution guidelines.
The pull request does not introduce breaking changes.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

SDK Generation Guidelines

The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code. (Track 2 only)
The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK. Please double check nuget.org current release version.

Additional management plane SDK specific contribution checklist:

Note: Only applies to Microsoft.Azure.Management.[RP] or Azure.ResourceManager.[RP]

Include updated management metadata.
Update AzureRP.props to add/remove version info to maintain up to date API versions.

Management plane SDK Troubleshooting

If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add new service label and/or contact assigned reviewer.
If the check fails at the Verify Code Generation step, please ensure:
- Do not modify any code in generated folders.
- Do not selectively include/remove generated files in the PR.
- Do use generate.ps1/cmd to generate this PR instead of calling autorest directly.
  Please pay attention to the @microsoft.csharp version output after running generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version,

Old outstanding PR cleanup

Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.

…e in swagger

check-enforcer · 2021-02-19T05:53:30Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them. In order to bootstrap pipelines for a new service, please perform following steps:

For data-plane/track 2 SDKs Issue the following command as a pull request comment:

/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run net - [service] - ci

For track 1 management-plane SDKs

Please open a separate PR and to your service SDK path in this file. Once that PR has been merged, you can re-run the pipeline to trigger the verification.

a-santamaria · 2021-02-19T19:45:53Z

adding managed clusters to eng\pipelines\mgmt.yml here: #18888

markcowl

Some comments inline. Also, to make your build and tests run in ci, you need to add a reference to your project here: https://github.com/Azure/azure-sdk-for-net/blob/master/eng/pipelines/mgmt.yml

markcowl · 2021-02-19T20:25:43Z

...viceFabricManagedClusters/src/Microsoft.Azure.Management.ServiceFabricManagedClusters.csproj

+		<PackageId>Microsoft.Azure.Management.ServiceFabricManagedClusters</PackageId>
+		<Description>Microsoft Azure Management ServiceFabricManagedClusters Library</Description>
+		<AssemblyName>Microsoft.Azure.Management.ServiceFabricManagedClusters</AssemblyName>
+		<Version>0.1.0</Version>


First version should be '1.0.0-beta.1'

markcowl · 2021-02-19T20:26:46Z

...viceFabricManagedClusters/src/Microsoft.Azure.Management.ServiceFabricManagedClusters.csproj

+		<Description>Microsoft Azure Management ServiceFabricManagedClusters Library</Description>
+		<AssemblyName>Microsoft.Azure.Management.ServiceFabricManagedClusters</AssemblyName>
+		<Version>0.1.0</Version>
+		<PackageTags>Microsoft Azure Management Library;ServiceFabricManagedClusters;ServiceFabricManagedClusters management;</PackageTags>


Also, need to add an 'Properties\AssemblyInfo.cs' file to provide metadata for the assembly (see other projects for specifics)

markcowl · 2021-02-19T20:28:09Z

....Azure.Management.ServiceFabricManagedClusters/tests/TestHelper/RecordedDelegatingHandler.cs

+    using System.Net.Http.Headers;
+    using System.Threading.Tasks;
+
+    public class RecordedDelegatingHandler : DelegatingHandler


Why are you adding this? There is already a similar delegating handler in the test framework

markcowl · 2021-02-19T20:31:36Z

...ters/Microsoft.Azure.Management.ServiceFabricManagedClusters/tests/Tests/ApplicationTests.cs

+                CreateAppType(serviceFabricMcClient, resourceGroupName, clusterName, cluster.ClusterId, AppTypeName);
+                var appTypeVersion = CreateAppTypeVersion(serviceFabricMcClient, resourceGroupName, clusterName, cluster.ClusterId, AppTypeName, AppTypeVersionName, AppPackageUrl);
+                CreateApplication(serviceFabricMcClient, resourceGroupName, clusterName, cluster.ClusterId, AppName, appTypeVersion.Id);
+                CreateService(serviceFabricMcClient, resourceGroupName, clusterName, cluster.ClusterId, AppName, StatefulServiceTypeName, StatefulServiceName);


There are no assertions over the properties of the returned types. It is generally a good idea to ensure that the serialization/deserialization of properties is working end-to-end in at east one test

thanks, I added properties validations

markcowl · 2021-02-19T20:39:09Z

...ft.Azure.Management.ServiceFabricManagedClusters/src/Generated/Models/ErrorModelException.cs

+// regenerated.
+// </auto-generated>
+
+namespace Microsoft.Azure.Management.ServiceFabricManagedClusters.Models


So, this is occurring because your spec is not referencing the common ARM error model (https://github.com/Azure/azure-rest-api-specs/blob/master/specification/common-types/resource-management/v1/types.json#L277). Using a custom error like this, rather than the exception form the management library will make it more difficult to handle errors uniformly across RP sdks for the customer

Ok. I understand thanks! we will fix this on the next swagger update

…validation in tests

a-santamaria · 2021-02-20T01:21:57Z

Some comments inline. Also, to make your build and tests run in ci, you need to add a reference to your project here: https://github.com/Azure/azure-sdk-for-net/blob/master/eng/pipelines/mgmt.yml

I created a separate pr #18888 as suggested by the check-enforcer comment. Also added it in this pr

a-santamaria · 2021-02-23T17:56:28Z

@markcowl I have addressed the comments, please review the update.

markcowl

One question about the service reference in the test project, otherwise, LGTM

...ricManagedClusters/tests/Microsoft.Azure.Management.ServiceFabricManagedClusters.Test.csproj

Alfredo Santamaria Gomez added 9 commits February 12, 2021 14:10

adding servicefabric managedclusters folder

84e7a17

generate managed clusters sdk

51881dc

generate sdk again with remove x-ms-client-flatten for ServiceResourc…

2c6fc08

…e in swagger

adding tests managed clsuters and apps

f0946e4

record tests

83b60b9

re-generate with CreateOrUpdate for apps

0e1fa71

change Create to CreateOrUpdate for apps test

cde2191

remove unnecessary subfolder and fix namespace

882bc0f

swagger pr merged. Generate from azure repo

1cccdbf

a-santamaria requested review from allenjzhang, bquantump, m-nash, markcowl, nisha-bhatia and YalinLi0312 as code owners February 19, 2021 05:48

openapi-assignment-bot bot assigned markcowl Feb 19, 2021

a-santamaria mentioned this pull request Feb 19, 2021

Add service fabric managed clusters to pipeline #18888

Closed

markcowl requested changes Feb 19, 2021

View reviewed changes

markcowl added the needs-revision label Feb 19, 2021

fix version, add AssemblyInfo, remove RecordedDelegatingHandler, add …

22976aa

…validation in tests

a-santamaria requested review from chidozieononiwu, danieljurek, mitchdenny and weshaggard as code owners February 20, 2021 01:18

a-santamaria removed the needs-revision label Feb 20, 2021

markcowl requested changes Feb 23, 2021

View reviewed changes

...ricManagedClusters/tests/Microsoft.Azure.Management.ServiceFabricManagedClusters.Test.csproj Outdated Show resolved Hide resolved

remove unnecessary include in test csproj

6e4323b

markcowl approved these changes Feb 24, 2021

View reviewed changes

markcowl merged commit 3370c21 into Azure:master Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service Fabric Managed Clusters - initial skd with api version 2021-01-01-preview #18875

Service Fabric Managed Clusters - initial skd with api version 2021-01-01-preview #18875

a-santamaria commented Feb 19, 2021

check-enforcer bot commented Feb 19, 2021

a-santamaria commented Feb 19, 2021 •

edited

Loading

markcowl left a comment

markcowl Feb 19, 2021

a-santamaria Feb 20, 2021

markcowl Feb 19, 2021

a-santamaria Feb 20, 2021

markcowl Feb 19, 2021

a-santamaria Feb 20, 2021

markcowl Feb 19, 2021

a-santamaria Feb 20, 2021

markcowl Feb 19, 2021

a-santamaria Feb 20, 2021

a-santamaria commented Feb 20, 2021

a-santamaria commented Feb 23, 2021

markcowl left a comment

Service Fabric Managed Clusters - initial skd with api version 2021-01-01-preview #18875

Service Fabric Managed Clusters - initial skd with api version 2021-01-01-preview #18875

Conversation

a-santamaria commented Feb 19, 2021

All SDK Contribution checklist:

Additional management plane SDK specific contribution checklist:

Management plane SDK Troubleshooting

Old outstanding PR cleanup

check-enforcer bot commented Feb 19, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

For data-plane/track 2 SDKs Issue the following command as a pull request comment:

For track 1 management-plane SDKs

a-santamaria commented Feb 19, 2021 • edited Loading

markcowl 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

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

a-santamaria commented Feb 20, 2021

a-santamaria commented Feb 23, 2021

markcowl left a comment

Choose a reason for hiding this comment

a-santamaria commented Feb 19, 2021 •

edited

Loading