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

[C#] Support Asp.net core 2.1 #999

Closed
SeanFarrow opened this issue Sep 9, 2018 · 39 comments
Closed

[C#] Support Asp.net core 2.1 #999

SeanFarrow opened this issue Sep 9, 2018 · 39 comments

Comments

@SeanFarrow
Copy link
Contributor

It would be nice if the generator supported Asp.Net Core 2.1.

I'm happy to do the work for this, but have a few questions:

Firstly, is there a way to completely exclude files from the output if they are not needed. The case where SwashBuckle is not included is the example I'm thinking of, so we should probably exclude wwroot.
Secondly, how does the generator indicate that authorization is needed on an endpoint to the code that generates files from the templates. I'm assuming the starting point of an API with no authorization specified.
Thanks for any help.
Sean.

Description
openapi-generator version
OpenAPI declaration file content or url
Command line used for generation
Steps to reproduce
Related issues/PRs
Suggest a fix/enhancement
@wing328
Copy link
Member

wing328 commented Sep 9, 2018

@SeanFarrow thanks for offering help to improve this project.

To exclude files, have you considered using .openapi-generator-ignore files to prevent files from being overwritten/generated?

I'm assuming the starting point of an API with no authorization specified.

Whether authorization is required depends on the OpenAPI specification. We can consider authorization as a day 2 requirement.

Have you tried the aspnetcore generator? Does the output meet your requirement?

We can add an option to support ASP.Net Core 2.1 if 2.1 does not introduce a lot of changes.

@wing328 wing328 added this to the 3.3.0 milestone Sep 9, 2018
@wing328 wing328 removed this from the 3.3.0 milestone Sep 9, 2018
@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 9, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 9, 2018

@SeanFarrow thanks for the details. Sounds like we should support 2.1 directly and drop the support for 2.0.

@jimschubert @mandrean do you have an option on this topic?

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 9, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 10, 2018

Perhaps we should allow a user to specify a version when generating the code?

Yup, that's usually what we do.

We can add an option "aspNetCoreVersion" to support future version (e.g. 2.2, 2.3, etc).

@wing328
Copy link
Member

wing328 commented Sep 10, 2018

@SeanFarrow as a starting point, what about updating the templates in https://github.com/OpenAPITools/openapi-generator/tree/master/modules/openapi-generator/src/main/resources/aspnetcore to make it work with ASP.net core 2.1 ?

@SeanFarrow
Copy link
Contributor Author

@wing328,
Yes, I'll do that. Do you want me to just update the template and then look at other things later, like the fact that if UseSwashbuckle is not specified, we don't need to generate some of the filters, so they shouldn't be present in the final output in my view.

@wing328
Copy link
Member

wing328 commented Sep 10, 2018

then look at other things later, like the fact that if UseSwashbuckle is not specified

Yup, we can deal with those later as it should not be difficult to skip with a CLI option.

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 10, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 10, 2018

Seems like there's no authorization support at the moment as I couldn't find the mustache tag authMethods in https://github.com/OpenAPITools/openapi-generator/tree/master/modules/openapi-generator/src/main/resources/aspnetcore

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 10, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 10, 2018

@SeanFarrow we can work with you to add the authorization support later (which should be pretty straightforward)

@SeanFarrow
Copy link
Contributor Author

@wing328,
Do we want to upgrade swashbuckle, the current version is 3.0.0 and we are currently referencing 2.4. It makes sense to upgrade everything if we are going to move frameworks. I'm assuming this will ship in v3.3.0 of the generator?

@wing328
Copy link
Member

wing328 commented Sep 10, 2018

Sure as Swashbuckle 3.0.0 also contains breaking changes: https://github.com/domaindrivendev/Swashbuckle.AspNetCore/releases/tag/v3.0.0

I'm assuming this will ship in v3.3.0 of the generator?

It's scheduled on coming Friday so we'll definitely include it if it's ready by that time.

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 10, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 10, 2018

To update the petstore sample, please run bin\windows\aspnetcore-petstore-server.bat at the root folder of this project.

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 10, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 10, 2018

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 10, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 10, 2018

Please take your time. Let us know if you need any help.

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 10, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 10, 2018

Do you want commits squashed into a single commit?

Nope, we'll squash your commits into one as part of the merge.

@jimschubert
Copy link
Member

To comment on this as far as ASP.NET Core 2.1, I'd prefer to just update fully to 2.1 and not leave support for 2.0. 2.1 is LTS while 2.0 is not, so I don't see a reason for us to maintain a 2.0 option. I wouldn't consider this a breaking change as the generator isn't currently written with an intention to regenerate over existing code.

.NET Core 2.0 will end-of-life at the end of this month.

@wing328
Copy link
Member

wing328 commented Sep 11, 2018

@jimschubert thanks for reviewing this.

After I reviewed #1008 (only changes to mustache templates), I would suggest adding an option to select the framework with ASP.NET core 2.1 being the default and we'll mark 2.0 as deprecated so that we can remove it in the future release. That way we can merge the enhancement into 3.3.x branch targeted to be released this Friday.

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 11, 2018 via email

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 11, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 11, 2018

@SeanFarrow I can add it after merging your PR (as I just did something similar for another enhancement related to Java Feign version upgrade)

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 11, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 11, 2018

@SeanFarrow sure, you will need to add a CLI option (e.g. aspNetCoreVersion) to support "2.0", "2.1" and put the templates (the existing one for v2.0) in a separate folder. We'd a very similar change for Dart to support Dart v2 recently and https://github.com/OpenAPITools/openapi-generator/pull/754/files#diff-79347eb38a35fa462a0c2b03aa771e75R129 is a good starting point.

Please let me know if you need help adding the option.

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 11, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 11, 2018

@SeanFarrow if we (you or me) add it after the minor release (3.3.x) this Friday, we may need to wait for 2 months as we plan to change the minor release schedule to bi-monthly: #956

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 11, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 11, 2018

No worry. I'll do it later today and keep you posted.

@wing328
Copy link
Member

wing328 commented Sep 11, 2018

I've updated #1008 with the option "aspnetCoreVersion" (default to 2.1). Please have a look when you've time.

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 12, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 12, 2018

@SeanFarrow PR merged. Thanks for your PR.

Do you have a Twitter account? We'll promote the enhancement via https://twitter.com/oas_generator

I'll update the documentation later if needed.

@SeanFarrow
Copy link
Contributor Author

SeanFarrow commented Sep 12, 2018 via email

@wing328
Copy link
Member

wing328 commented Sep 12, 2018

👌

@wing328 wing328 closed this as completed Sep 12, 2018
@wing328
Copy link
Member

wing328 commented Sep 12, 2018

Tweet: https://twitter.com/oas_generator/status/1039932534258167808

Please help retweet to promote the enhancement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants