use IsDevelopment

[Rick-Anderson]

See https://github.com/dotnet/aspnetcore-internal/issues/4522

• [ x] You've read the Contributor Guide and Code of Conduct.
• [x ] You've included unit or integration tests for your change, where applicable.
• [ x] You've included inline docs for your change, where applicable.
• [ x] 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.

Summary of the changes: see Files changed

Description

see Files changed

contributes to https://github.com/dotnet/aspnetcore-internal/issues/4522

[commonsensesoftware]

@Rick-Anderson what is the rationale behind this PR? I can't see the internal notes so I'm not entirely sure what the context is. I see that the change just conditionally adds whether the Swagger UI is added. I understand why it would conditional in a template. This example is explicitly meant to show the SwaggerUI. I'm trying to determine when you wouldn't want that enabled. If this were to be conditional, then all of the examples that can show the SwaggerUI should be brought into alignment; at least, the ASP.NET Core examples. I honestly don't remember how or if that conditional setup was possible back in Web API.

[Rick-Anderson]

@Rick-Anderson what is the rationale behind this PR? I can't see the internal notes so I'm not entirely sure what the context is. I see that the change just conditionally adds whether the Swagger UI is added. I understand why it would conditional in a template. This example is explicitly meant to show the SwaggerUI. I'm trying to determine when you wouldn't want that enabled.

@captainsafia asked me to make this change.

If this were to be conditional, then all of the examples that can show the SwaggerUI should be brought into alignment; at least, the ASP.NET Core examples. I honestly don't remember how or if that conditional setup was possible back in Web API.

That's why this is a draft PR, I'll make them all consistent.

@commonsensesoftware thanks for the comments and hopefully you'll review my changes when this is out of draft.

[captainsafia]

@captainsafia asked me to make this change.

Yep, to add some color to this, we're trying to standardize around the best practice of only enabling Swagger UI for development scenarios across samples/docs in the repo. We've done this with the eShop repo and have a few PRs to make our docs pages consistent here.

WRT to why only enabling it in development is a best practice, there's some information and secret disclosure concerns with Swagger UI in production scenarios so we want to guide users towards the most conservative/secure option by default. There's also the challenge that the middleware-based approach for Swagger UI is a little bit hard to enable authentication on to reduce information disclosure risks.

For documentation and samples where Swagger UI is primarily used to aid with ad-hoc dev-time testing and exploration of APIs, it helps to be explicit with the isDevelopment check.

[commonsensesoftware]

I don't really have an objection; it just came out of the blue. 😉 I know showing the Swagger UI outside of development is not typically a best practice. The examples in question were explicitly meant to show the Swagger UI (all the time) and I had removed all other cruft to dial into what really matters.

I do, however, prefer and try to make examples represent the real world and what you ought to do. It's very unlikely anyone would ever run these examples in a Release build, so I don't have a problem aligning to illustrate the right thing. It's not going to affect the default behavior to clone, build, and run.

[commonsensesoftware]

Speaking of the eShop repo @captainsafia, I'm still waiting on that PR review to integrate API Versioning as you had asked. 😁 I believe I've fixed or answered all your feedback. There was some other feedback about making the rendered information more ergonomic if it's present. There currently isn't any such information for the eShop repo to see what it would look like, but I've already ported it back here in the latest examples. If you pull and run any of the OpenAPI examples, you can see what it'll look like.

[captainsafia]

Speaking of the eShop repo @captainsafia, I'm still waiting on that PR review to integrate API Versioning as you had asked. 😁 I believe I've fixed or answered all your feedback.

Yes, I'm so sorry it's taken me so long to get back to you! 😓 I've been heads down working on landing OpenAPI doc generation support for preview4 and have had to be more focused with my time. I hope to take another look by middle of next week. I specifically want to make sure that the new OpenAPI work + the ASP versioning changes will connect well once we update eShop to target the built-in OpenAPI implementation

[commonsensesoftware]

@captainsafia Happy to carve out some time and have a discussion about it some time. I'm also happy to chime in or comment on anything that needs review or is ready to vet (if it's before preview). Feel free to email me. I've already come to terms with the fact it will likely be a full rewrite and new library/package on my part. It would be great to align for the Nov release.

[Rick-Anderson]

@commonsensesoftware please review.

[Rick-Anderson]

@mikekistler please review

[Rick-Anderson]

@commonsensesoftware can you merge this?