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.

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

[Resources] Add deployment scripts resource type to the Resources SDK #8436

Merged
merged 11 commits into from
Nov 8, 2019

Conversation

filizt
Copy link
Contributor

@filizt filizt commented Oct 25, 2019

The new preview version of Resources SDK will support deployment scripts resource type. This new resource type is added to the Resources namespace.

Link to the swagger PR: Azure/azure-rest-api-specs#7486

Question for the reviewer

There was a name clash while generating the Page.cs - I renamed our Page.cs file temporarily. I see that there are already two separate page classes in the SDK (see Page.cs and Page1.cs) Should we rename our page class to Page2? If so, is there a automated way of doing it through Autorest(AFAIK there is none) or should we do it manually? I would think doing it manually will create extra work in the future if/when we re-generate the SDK since we need to rename the class again. It'd be good to know if this can be done through Autorest.

Another option would be using the class that is already in the project. Though this also might not be ideal since we'll be tying our code to a class that is being generated by some other swagger.

What would be a good way to solve this issue? Your help in this matter is appreciated.

@filizt filizt requested a review from erich-wang as a code owner October 25, 2019 15:14
@AlexGhiondea AlexGhiondea added the Mgmt This issue is related to a management-plane library. label Oct 26, 2019
@isra-fel isra-fel self-assigned this Oct 28, 2019
@isra-fel
Copy link
Member

@AlexGhiondea @kurtzeborn Hi, resources team met something like an autorest bug. Could you help investigate? Thanks

@isra-fel
Copy link
Member

isra-fel commented Oct 31, 2019

Hi @filizt , you'll need the following things:

  • Add a link to the swagger spec review PR in the description
  • Include the .txt file generated when you run generate.ps1(details)
  • Update versions, tags, if necessary
  • Fix existing tests and write new ones to cover new code
    Thanks

@AlexGhiondea
Copy link
Contributor

@erich-wang could you help with this?

@fearthecowboy
Copy link
Member

Where is the spec that contains the schema that is called page?

@filizt
Copy link
Contributor Author

filizt commented Nov 1, 2019

Where is the spec that contains the schema that is called page?

Here's the swagger that we use to generate our SDK. https://github.com/Azure/azure-rest-api-specs/blob/master/specification/resources/resource-manager/Microsoft.Resources/preview/2019-10-01-preview/deploymentScripts.json

I believe the page.cs gets created by the autorest since we have a pagable list call:

"x-ms-pageable": { "nextLinkName": "nextLink" }

@fearthecowboy
Copy link
Member

Ok, I'm thoroughly confused here.

Are you saying the generator is creating a third Page.cs class in addition to the Page and Page1 classes in Models?

Where does the extra Page.cs that you have to delete come from?

Another option would be using the class that is already in the project. Though this also might not be ideal since we'll be tying our code to a class that is being generated by some other swagger.

The Page.cs class that generated by the c# generator will be the same no matter which swagger file created it.

And given that no new work is going into that generator, it's not going to change.

@filizt
Copy link
Contributor Author

filizt commented Nov 6, 2019

Hi @filizt , you'll need the following things:

  • Add a link to the swagger spec review PR in the description
  • Include the .txt file generated when you run generate.ps1(details)
  • Update versions, tags, if necessary
  • Fix existing tests and write new ones to cover new code
    Thanks

@isra-fel I have included the generated .txt file, updated versions, and tags. Could you see if I'm missing anything?

I'm recording tests and will submit them soon.

@filizt
Copy link
Contributor Author

filizt commented Nov 6, 2019

Ok, I'm thoroughly confused here.

Are you saying the generator is creating a third Page.cs class in addition to the Page and Page1 classes in Models?

Where does the extra Page.cs that you have to delete come from?

Another option would be using the class that is already in the project. Though this also might not be ideal since we'll be tying our code to a class that is being generated by some other swagger.

The Page.cs class that generated by the c# generator will be the same no matter which swagger file created it.

And given that no new work is going into that generator, it's not going to change.

@fearthecowboy Sorry for the confusion. Let me try to elaborate.

I think the root cause of my question is coming from the fact that there are two Page files in Models in the Microsoft.Resources SDK: Page.cs and Page1.cs. I'm curious to find out why that is and if autorest tool created these two separate Page files.

Here're the Page files I am referring to: https://github.com/Azure/azure-sdk-for-net/tree/master/sdk/resources/Microsoft.Azure.Management.Resource/src/Generated/Models

@fearthecowboy
Copy link
Member

The C# generator did create both of those.

I'm not entirely sure why it does that. It's not going to affect anything, there's not any reason to do anything with them.

@filizt
Copy link
Contributor Author

filizt commented Nov 7, 2019

Hi @filizt , you'll need the following things:

  • Add a link to the swagger spec review PR in the description
  • Include the .txt file generated when you run generate.ps1(details)
  • Update versions, tags, if necessary
  • Fix existing tests and write new ones to cover new code
    Thanks

@isra-fel I have included the generated .txt file, updated versions, and tags. Could you see if I'm missing anything?

I'm recording tests and will submit them soon.

Submited the test recordings. Also updated the version number as per out discussion. Is there anything else missing from the PR?

@isra-fel
Copy link
Member

isra-fel commented Nov 7, 2019

Submited the test recordings. Also updated the version number as per out discussion. Is there anything else missing from the PR?

Looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants