Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding Maintenance RP .NET SDK, ARM Swagger PR https://github.com/Azure/azure-rest-api-specs/pull/6469 #7485

Merged
merged 8 commits into from
Sep 11, 2019
Merged

Adding Maintenance RP .NET SDK, ARM Swagger PR https://github.com/Azure/azure-rest-api-specs/pull/6469 #7485

merged 8 commits into from
Sep 11, 2019

Conversation

abkmr
Copy link
Contributor

@abkmr abkmr commented Sep 5, 2019

Adding Maintenance RP .NET SDK
Approved by ARM

@abkmr abkmr requested a review from erich-wang as a code owner September 5, 2019 21:12
@isra-fel isra-fel added Mgmt This issue is related to a management-plane library. needs-review labels Sep 6, 2019
@isra-fel isra-fel self-assigned this Sep 6, 2019
@isra-fel
Copy link
Member

isra-fel commented Sep 6, 2019 •
edited
Loading

Hi @abkmr, the SDK should be placed in /sdk/{moduleinlowercase}. I also notice that you missed the metadata file. Maybe you didn't follow these steps when generating? 🤔

Besides, could you share the link to swagger review PR(s), thanks?

@isra-fel isra-fel assigned abkmr and unassigned isra-fel Sep 6, 2019
@abkmr abkmr changed the title Adding Maintenance RP .NET SDK Adding Maintenance RP .NET SDK, ARM Swagger PR https://github.com/Azure/azure-rest-api-specs/pull/6469 Sep 7, 2019
@abkmr
Copy link
Contributor Author

abkmr commented Sep 7, 2019

Hi @abkmr, the SDK should be placed in /sdk/{moduleinlowercase}. I also notice that you missed the metadata file. Maybe you didn't follow these steps when generating? 🤔

Besides, could you share the link to swagger review PR(s), thank

I had followed that but did not remember to add metadata file. PR for swagger is Azure/azure-rest-api-specs#6469. Updated the casing for rp. thanks

isra-fel
isra-fel suggested changes Sep 7, 2019
View reviewed changes
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @abkmr , please

  • check in AssemblyInfo.cs
  • write tests for SDK code

and please see my other inline comments. Thanks!

src/SDKs/maintenance/Management.Maintenance/Microsoft.Azure.Management.Maintenance.csproj Outdated Show resolved Hide resolved
src/SDKs/_metadata/maintenance_resource-manager.txt Outdated
@@ -0,0 +1,14 @@
Installing AutoRest version: latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is in the wrong path, it should be in eng\mgmt\mgmtmetadata\. Could you run

msbuild eng/mgmt.proj /t:Util /p:UtilityName=InstallPsModules

then re-run generate.ps1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like my repo fork was done when all RPs had Caps folder names, and the eng\mgmt.proj did not exist. I did a merge from master repo and will update.

@abkmr
Copy link
Contributor Author

abkmr commented Sep 10, 2019

Hi @abkmr , please

  • check in AssemblyInfo.cs
  • write tests for SDK code

and please see my other inline comments. Thanks!

Added tests and AssemblyInfo.cs

@abkmr abkmr requested a review from isra-fel September 11, 2019 08:21
@isra-fel isra-fel assigned isra-fel and unassigned abkmr Sep 11, 2019
@isra-fel isra-fel merged commit ed45d2b into Azure:master Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isra-fel isra-fel isra-fel approved these changes

@erich-wang erich-wang Awaiting requested review from erich-wang

Assignees

@isra-fel isra-fel

Labels
Mgmt This issue is related to a management-plane library.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@abkmr @isra-fel