Skip to content

Add Microsoft.AspNetCore.OpenApi to the webapiaot template (with an optional flag to disable it) #60337

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

Merged
merged 22 commits into from
Mar 11, 2025

Conversation

sander1095
Copy link
Contributor

@sander1095 sander1095 commented Feb 12, 2025

Add Microsoft.AspNetCore.OpenApi to the webapiaot template (with an optional flag to disable it)

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Creates feature parity with the webapi template in regards to OpenAPI

Description

This PR adds Microsoft.AspNetCore.OpenApi to the dotnet new webapiaot template as .NET 9 made that package AOT compatible. Just like the webapi template, OpenAPI support is included by default and it now also contains a --no-openapi flag to disable OpenAPI if desired.

Fixes #59564

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Feb 12, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 12, 2025
@sander1095
Copy link
Contributor Author

sander1095 commented Feb 12, 2025

This PR still has the following issues:

  • I'm having issues following the README to execute the tests successfully
    • ./restore.sh
      source ./activate.sh
      (aspnetcore) sander@PC-Sander:~/projects/aspnetcore/src/ProjectTemplates$ bash build.sh -test -configuration Release
      
      This just doesn't want to work, not on Windows, not on WSL (Ubuntu 24). I'm following instructions perfectly. I expect it's an issue on my end.
  • I still need to create a new project based on these changes and test things out, which I can't get to work (even with existing projects on main) if I follow each step of the README.
  • The "get by id" endpoint needs more metadata to return the NotFound result

I'd love some help/instructions with the issues mentioned above! If someone could verify the package works, that'd be great..

@sander1095 sander1095 marked this pull request as ready for review February 12, 2025 13:47
@sander1095 sander1095 marked this pull request as draft February 12, 2025 13:48
…ration Release`.

I assume this is part of the natural process of creating a PR and thus should be committed.
@captainsafia
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@captainsafia
Copy link
Member

@sander1095 This looks good to me overall!

I'd like to get @DamianEdwards thoughts on this since he's got more experience with making template changes and might have thoughts on other things we need to do here. He's OOF right now so we might have to wait a while for his feedback.

@Rick-Anderson
Copy link
Contributor

@DamianEdwards can you schedule time for review?

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 28, 2025
@captainsafia
Copy link
Member

/azp run

@captainsafia captainsafia enabled auto-merge (squash) February 28, 2025 23:29
@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 28, 2025
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@sander1095
Copy link
Contributor Author

Hi @DamianEdwards ! Thanks for approving this PR. Safia said:

I'd like to get @DamianEdwards thoughts on this since he's got more experience with making template changes and might have thoughts on other things we need to do here. He's OOF right now so we might have to wait a while for his feedback.

Like I said in my comments in this PR, I can not get the tests to run correctly, nor can I create a project to test my changes locally.

Are you able to help out with this?

@captainsafia
Copy link
Member

@sander1095 Absent running tests locally, we can rely on test failiures in CI to diagnose issues. Based on the build analysis, it seems like the templates are currently unbuildable because the correct usings for the TypedResult static methods are not in place.

auto-merge was automatically disabled March 3, 2025 20:43

Head branch was pushed to by a user without write access

@sander1095
Copy link
Contributor Author

sander1095 commented Mar 3, 2025

@sander1095 Absent running tests locally, we can rely on test failiures in CI to diagnose issues. Based on the build analysis, it seems like the templates are currently unbuildable because the correct usings for the TypedResult static methods are not in place.

Apologies, I didn't know there was such a handy UI with the explicit errors.

I've researched this, and I found that ImplicitUsings for ASP.NET Core projects does not add Microsoft.AspNetCore.Http.HttpResults to the list of global usings.

This means we need to make a choice:

  1. Either we add the Microsoft.AspNetCore.Http.HttpResults using explicitly (which I've just done in this PR, so we can at least see how much the the pipeline progresses)
  2. Add Microsoft.AspNetCore.Http.HttpResults to the list of global usings, which can be done in the SDK repo. I've already created a draft PR for this: Add HttpResults to the ImplicitUsings of ASP.NET Core projects so TypedResults/Results<T1,T2> don't need an explicit using sdk#47231

I'd prefer option 2 so we can keep the Program.cs file cleaner for new projects, and to remove an explicit using for existing projects. Of course this would mean that the SDK PR would need to reviewed and merged first before this PR can be finally merged.

What do you think, @captainsafia ? If you also agree with option 2, I can remove the draft status of the PR in the SDK repo

@captainsafia
Copy link
Member

What do you think, @captainsafia ? If you also agree with option 2, I can remove the draft status of the PR in the SDK repo

I think for this case, we can stick to explicit usings in the Program.cs for this templates. Modifying the global usings is a more global (heh) change to be mindful and it's harder to take back once we've done it compared to a local change in a single template.

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

LGTM! TY for adding tests and resolving all the details.

I kicked CI -- looked like a flake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"dotnet new webapiaot" should include OpenAPI support
4 participants