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

Review of changes to ANCM Configuration Reference #2206

Closed
guardrex opened this issue Nov 18, 2016 · 14 comments
Closed

Review of changes to ANCM Configuration Reference #2206

guardrex opened this issue Nov 18, 2016 · 14 comments
Milestone

Comments

@guardrex
Copy link
Collaborator

@Rick-Anderson @spboyer @Tratcher @pranavkm @shirhatti @moozzyk @danroth27

A significant update was just made to the ASP.NET Core Module Configuration Reference to accommodate the new ANCM overview doc, but I see just a handful of very minor grammatical issues.

Instead of submitting a PR outright, I'll post on this issue the handful of suggestions that I have for approval if that works for you.

@guardrex guardrex changed the title Review of changes to ASP.NET Core Module Configuration Reference Review of changes to ANCM Configuration Reference Nov 18, 2016
@Rick-Anderson
Copy link
Contributor

@guardrex OK

@guardrex
Copy link
Collaborator Author

guardrex commented Nov 18, 2016

If you like any of these, I'll get a PR together this weekend. I numbered them for reference.


1

ASP.NET Core Module is an IIS module that lets Kestrel use IIS or IIS Express as a reverse proxy server.

I recommend "allows Kestrel to use" (more formal) over "lets Kestrel use" (somewhat informal).


2

The Web.config file

Throughout .NET Core docs and samples, Web.config (the uppercase IIS clan file naming convention) is ignored in favor of the lowercase filename, web.config. Throughout this doc and in all of the samples the lowercase filename predominates. For consistency, I think it should be lowercase here. Conversely, it could be uppercase everywhere else.

Also, this section doesn't strike me as really addressing web.config, per se. It's about using web.config to configure the module. What do you think of Configuration via web.config?


3

Here's an example from a Visual Studio project, with placeholders for settings that Visual Studio plugs in when you run or debug the project.

[EXAMPLE]

When the project is published, Visual Studio automatically plugs in the values required for the destination environment, as shown in this example of a project that has been deployed to an Azure web app.

[EXAMPLE]

I'm surprised this was added at this time, as it seems to apply to VS2017, MSBuild/csproj, and utility provided by Microsoft.NET.Sdk.Web. There is also a comma splice btw ("project, with").

Would you like this refactored to address the use of publish-iis tooling? ... or dropped? ... or left alone (except for the comma splice)?

Side note: This section has been introduced here for the first time. Since we've seen a number of devs include the <handlers> section in subapps, I recommend to say something about removing the <handlers> section in subapp web.config files. It's a common problem, and a few words here will probably save a number of dev headaches.


4

Duration in seconds for which the module will wait for the executable to start a process listening on the port.

... change "for which" to "that" ...

Duration in seconds that the module will wait for the executable to start a process listening on the port.


5

Here's an example that sets two environment variables.

There are three env vars set now, so we'd probably remove the ENV_VAR_2 setting.

I don't have a hard suggestion ... just a question on this: It seems a little strange to me to show the ASPNETCORE_ENVIRONMENT env var as an example. web.config transforms aren't supported at this time (aspnet/Tooling#252), and this implies that devs will be creating alternate web.config files and having them ✨ auto-magically ✨ get picked up. It might imply that this is the correct/best way to configure the environment, which I think many would find objectionable.

Would it make more sense right now to use ASPNETCORE_PORT or something like ASPNETCORE_DETAILEDERRORS here? Would it be better to just remove the ASPNETCORE_ENVIRONMENT entry from the example and leave it with the two generic env vars I listed? ... or how about something more generally relevant, such as a suggestion by @wpostma for CONFIG_DIR, which is how he picks up his config file directory in Startup?


6

Look at the HTTP Errors attribute to override this page with a custom error page.

to

For more information on configuring custom error messages, see HTTP Errors <httpErrors>.


7
Add backticks to "aspNetCore" in ...

Here's a sample aspNetCore element that configures stdout logging

... and explicitly specify that this example is for Azure Apps.

@wpostma
Copy link

wpostma commented Nov 18, 2016

ASP.NET Core Module is an IIS module that lets Kestrel use IIS or IIS Express as a reverse proxy server.

I recommend "allows Kestrel to use" (more formal) over "lets Kestrel use" (somewhat informal).

Um. Can I just say I find that confusing, it seems like passive voice. Kestrel is designed to be fast, not designed to be secure, or load balanced, or to have policies or management or lots of logging. So IIS is a production web hosting platform, and kestrel is an ultra lightweight http request server.

So I would say "ASP.NET Core Module is the module that lets you run portable .net core applications behind IIS, using IIS for what it's good at (security, manageability, and lots more) and using Kestrel for what it's good at (being really really fast), and getting the benefits from both technologies at once".

Users don't actually know or care what a Reverse Proxy Server is. Why bother explaining that at the introductory material? This is the thing that let's you have peanut butter and chocolate, and lets them be friends.

@guardrex
Copy link
Collaborator Author

Technically tho, the description is correct. This is merely the reference doc. I think the intention is that general info, descriptions, how-to is all in the new ANCM overview doc at https://docs.microsoft.com/en-us/aspnet/core/fundamentals/servers/aspnet-core-module.

@Rick-Anderson
Copy link
Contributor

@wpostma can't we use your approach but also mention that you get security from the RPS. We can't leave out that detail.

@guardrex looks good, go ahead.

@wpostma
Copy link

wpostma commented Nov 18, 2016

Oh sure, I'm just giving an End User perspective here. What I first found confusing about ANCM is that it exists at all. Then I realized, this is a more modern architecture than ISAPI or DCOM, so of course it's a reverse proxy.

@guardrex
Copy link
Collaborator Author

guardrex commented Nov 18, 2016

@Rick-Anderson 👍 ... but on specifics ...

RE: 3 ... VS modding the web.config: Would you like this refactored to address the use of publish-iis tooling? ... or left alone (except for the comma splice)? I assume you don't want it dropped, since the ANCM overview specifically says to come here for a web.config example.

RE: 5 ... the env var example: Leave it alone? ... change it to ASPNETCORE_PORT or ASPNETCORE_DETAILEDERRORS (or both)? ... or remove ASPNETCORE_ENVIRONMENT and leave the generic ones (or better generic ones) in place?

☝️ or do you want me to make the call? In which case, I'll go with publish-iis language and change the env var example to ASPNETCORE_PORT.

@wpostma I recommend you float that over to @tdykstra for https://docs.microsoft.com/en-us/aspnet/core/fundamentals/servers/aspnet-core-module? @Rick-Anderson, what we could do here is just drop the first sentence entirely. This whole doc can just start with the second sentence.

Although, I would combine it with the next line. Like Captain Kirk, "I don't believe in the one-line paragraph." 😄 [well ... I try to avoid them most of the time]

I would also make a few other minor course corrections, but this doc would open well with something like ...

This document provides details on how to configure the ASP.NET Core Module for hosting ASP.NET Core applications. For an introduction to the ASP.NET Core Module and installation instructions, see the ASP.NET Core Module overview.

[ 🎤 drop ] 😄

@Rick-Anderson Rick-Anderson added this to the 1.0.1 milestone Nov 18, 2016
@wpostma
Copy link

wpostma commented Nov 18, 2016

That's a good page. Why not link to that page from here? Links are awesome. People will google and land all over your docs.

@Rick-Anderson
Copy link
Contributor

@guardrex you make the call.

@Tratcher
Copy link
Member

ASPNETCORE_ENVIRONMENT was my suggestion. It's a real value that people ask how to set. They're likely to set this after publishing as a way to help them debug their app.

@guardrex
Copy link
Collaborator Author

guardrex commented Nov 18, 2016

@Tratcher Ok ... I'll leave that.

Why not link to that page from here?

@wpostma It is ... it will be in the opening paragraph.

@tdykstra
Copy link
Contributor

tdykstra commented Nov 18, 2016

For the fundamentals doc, @wpostma intro sentence suggestion sounds good -- I'd just make minor changes:

ASP.NET Core Module lets you run ASP.NET Core applications behind IIS, using IIS for what it's good at (security, manageability, and lots more) and using Kestrel for what it's good at (being really fast), and getting the benefits from both technologies at once.

For this doc I like @guardrex suggestion

This document provides details on how to configure the ASP.NET Core Module for hosting ASP.NET Core applications. For an introduction to the ASP.NET Core Module and installation instructions, see the ASP.NET Core Module overview.

Regarding "allow" vs. "lets", the choice of the latter results from an effort on Microsoft's part to make its docs sound less formal. I was accustomed to using "allow" in cases like this but have been advised that the "lets" language is preferred for that reason.

Suggestions 2, 4, 6, 7 all sound fine

@guardrex
Copy link
Collaborator Author

guardrex commented Nov 21, 2016

@Rick-Anderson I see your merge requests are going against "master;" but when I did it, it went against "aspnet:master." That seemed to have hung both PR's. Where did I go wrong there?

Also, curious how I can view the build progress as before? There's no link in the vicinity of ...

capture

[EDIT] I resubmitted the PR's. They went through this time.

@Rick-Anderson
Copy link
Contributor

@guardrex sometimes the OPS build doesn't show up. It usually shows up. If you see that again, let me know. I usually close/reopen and that fixes it.

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

No branches or pull requests

5 participants