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.

Sign up for GitHub

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

Jump to bottom

Missing a step #6523

Closed
rjamesnw opened this issue May 22, 2018 — with docs.microsoft.com · 15 comments
Closed

Missing a step #6523

rjamesnw opened this issue May 22, 2018 — with docs.microsoft.com · 15 comments
Assignees
@scottaddie
Labels
Source - Docs.ms Docs Customer feedback via GitHub Issue
Milestone
Sprint 136 (5/19/...

Comments

@rjamesnw docs.microsoft.com
Copy link

rjamesnw commented May 22, 2018 •
edited
Loading

Nothing was working until I found an example that added a required line to the startup constructor:

if (env.IsDevelopment())
{
    builder.AddUserSecrets();
}

You might want to add that vital piece of info. :/

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.
@guardrex
Copy link
Collaborator

This probably happened because it was assumed that CreateDefaultBuilder would be in use, which adds it auto-magically ...

https://github.com/aspnet/MetaPackages/blob/dev/src/Microsoft.AspNetCore/WebHost.cs#L165-L172

As you suggest @rjamesnw, that's not a good idea. I'll add a paragraph to cover it. Thanks for bringing this to our attention.

@guardrex guardrex self-assigned this May 22, 2018
@guardrex guardrex added the Source - Docs.ms Docs Customer feedback via GitHub Issue label May 22, 2018
@guardrex guardrex mentioned this issue May 22, 2018
Closed
@guardrex guardrex removed their assignment May 22, 2018
@guardrex
Copy link
Collaborator

I just discovered this was recently addressed by #6488. Thanks @rjamesnw for mentioning it. The new content should go live shortly.

@guardrex guardrex self-assigned this May 22, 2018
@guardrex
Copy link
Collaborator

@rjamesnw We discussed it, and we're going add a hair more info to call out how CreateDefaultBuilder calls AddUserSecrets under-the-covers and that if CreateDefaultBuilder isn't called when building the host that the dev should then do as you suggest ☝️ on their own.

@guardrex guardrex reopened this May 22, 2018
@guardrex guardrex mentioned this issue May 22, 2018
Merged
scottaddie pushed a commit that referenced this issue May 22, 2018
@guardrex @scottaddie
App secrets update (#6528)
b9c408f 
Fixes #6523

[Internal Review Topic](https://review.docs.microsoft.com/en-us/aspnet/core/security/app-secrets?view=aspnetcore-1.0&branch=pr-en-us-6528)

Tiny nits in the app secrets update: "or later" and a couple of comma splices.

What I wrote for another version of this was more explicit. I'm concerned that the current version doesn't tell devs exactly why/how `AddUserSecrets` is added, and it doesn't say that if `CreateDefaultBuilder` isn't used that they'll still need to do it manually.

Starting with ...

> \- Add the user secrets configuration source to the `Startup` constructor:

> Add the user secrets configuration source with a call to \[AddUserSecrets](/dotnet/api/microsoft.extensions.configuration.usersecretsconfigurationextensions.addusersecrets) in the `Startup` constructor:
>
> \[!code-csharp\[](app-secrets/samples/1.1/UserSecrets/Startup.cs?name=snippet_StartupConstructor&highlight=5-8)]
> ::: moniker-end
> ::: moniker range=">= aspnetcore-2.0"
> When a project calls \[CreateDefaultBuilder](/dotnet/api/microsoft.aspnetcore.webhost.createdefaultbuilder) to initialize a new instance of the host with preconfigured defaults, \[AddUserSecrets](/dotnet/api/microsoft.extensions.configuration.usersecretsconfigurationextensions.addusersecrets) is called when the \[EnvironmentName](/dotnet/api/microsoft.aspnetcore.hosting.ihostingenvironment.environmentname) is \[Development](/dotnet/api/microsoft.aspnetcore.hosting.environmentname.development):
>
> \[!code-csharp\[](app-secrets/samples/2.1/UserSecrets/Program.cs?name=snippet_CreateWebHostBuilder&highlight=2)]
>
> When `CreateDefaultBuilder` isn't called during host construction, add the user secrets configuration source with a call to \[AddUserSecrets](/dotnet/api/microsoft.extensions.configuration.usersecretsconfigurationextensions.addusersecrets) in the `Startup` constructor:
>
> \[!code-csharp\[](app-secrets/samples/1.1/UserSecrets/Startup.cs?name=snippet_StartupConstructor&highlight=5-8)]
> ::: moniker-end

Snippet the call to `CreateDefaultBuilder` to show it (line 2 is highlighted) ...

```csharp
#region snippet_CreateWebHostBuilder
public static IWebHostBuilder CreateWebHostBuilder(string[] args) =>
    WebHost.CreateDefaultBuilder(args)
        .UseStartup<Startup>();
#endregion
```

... and the sample naming issue remains ("1.1" and "2.1" instead of "1.x" and "2.x").

If anything here looks good, I can mod this branch to make additional updates. Otherwise, nevermind. Just spit-ball'in here.
@rjamesnw
Copy link
Author

rjamesnw commented May 22, 2018 •
edited
Loading

Also you need to confirm if the secrets tool is really bundled. I have the latest SDK installed and it is NOT bundled by default for new projects (as your help site claims). I had to add DotNetCliToolReference to my project file else the command line wouldn’t work. Also tool version 2.0.2 is required with Core 2.0 so you need to add that also.

@guardrex
Copy link
Collaborator

It was covered here 👉 https://docs.microsoft.com/aspnet/core/security/app-secrets?view=aspnetcore-2.0&tabs=windows#install-the-secret-manager-tool. The likely reason that 1.0.1 is shown and not a 2.0 approach is that we expect all devs to upgrade from 2.0 to 2.1, thus all devs will get global tool installs with 2.1. That's the plan AFAIK anyway. @scottaddie Do you have additional thoughts?

@rjamesnw
Copy link
Author

rjamesnw commented May 22, 2018 •
edited
Loading

You totally missed my point. I know it is there, I even alluded to it. I’m telling you I have 2.1 installed and the tool is NOT available anywhere until I “install” the Nugget package into my project. Is it stands, following the steps on the page for 2.1 like I did does not supply any tools at all.

@guardrex
Copy link
Collaborator

guardrex commented May 22, 2018 •
edited
Loading

the latest SDK

Yes, I see what you mean now. The "latest (released) SDK" is 2.0 ... threw me off for a sec.

Idk if it will help, but they're working on the PR to cover global tools here 👉 dotnet/docs#5036 ... there might be some troubleshooting/gotcha info in their topic(s) on that PR. [EDIT] For example, feeds can be a problem to address with the pre-release bits.

Idk on the 2.1 SDK not installing the Secret Manager tool specifically. Scott will be along at some point and may be able to help.

@guardrex guardrex reopened this May 22, 2018
@scottaddie
Copy link
Member

@natemcmaster If using the .NET Core 2.1 SDK with an ASP.NET Core 2.0 app, I assume the <DotNetCliToolReference /> node is needed in the *.csproj file. Thoughts?

@natemcmaster
Copy link
Contributor

Unfortunately, the "2.1 SDK" is a bit ambiguous1. You will need the <DotNetCliToolReference /> if and only if dotnet --version < 2.1.300. The important bit is the third part of that version number. The latest stable SDK is 2.1.200, which does not have dotnet user-secrets bundled yet. We're expecting to ship 2.1.300 soon, but it's still only available as an RC.. Once you upgrade to 2.1.300 or newer, you will not need the <DotNetCliToolReference />.

Also, it's important to note that this is independent of the version of ASP.NET Core. You can use the 2.1.300 SDK for any version of ASP.NET, all the way back to 1.0.

1Starting with the 2.1.300 SDK, we plan to make sure the major.minor version of the SDK matches the latest .NET Core runtime it bundles. See dotnet/designs#29

@guardrex
Copy link
Collaborator

@scottaddie That covers most of the problem, but I'm concerned about this ...

For .NET Core SDK 2.0 and earlier, tool installation is necessary.

https://docs.microsoft.com/aspnet/core/security/app-secrets#install-the-secret-manager-tool

... leading right into an example with a 1.x era tool install.

We probably don't care too much about 2.0 because our general plan is to cover the latest 2.x release (i.e., 2.1). For that line to call out "SDK 2.0" given what @natemcmaster just said is of concern. We should consider either spelling out all of the caveats of the 2.0-era (yuck! 😓) or make that line read ...

For .NET Core SDK 1.1 and earlier, tool installation is necessary.

The sentence immediately prior to that ...

The Secret Manager tool is bundled with the .NET Core CLI in .NET Core SDK 2.1.

... is good. 👍 We can leave it at that given that we want devs to choose 2.1 over 2.0.

Whatever we do, I think we should moniker-range the content coverage.

@rjamesnw
Copy link
Author

rjamesnw commented May 23, 2018 •
edited
Loading

That’s fine, but it would have saved me lots of time to see a note at least that “tool installation is necessary if you have 2.1.200 or earlier” since I DID have it, and many others in company environments may also have it and not able to update on the bleeding edge. ;)

@guardrex
Copy link
Collaborator

Yes, but 2.1 won't be bleeding edge on May 30. I defer to @scottaddie and @Rick-Anderson how much 2.0 coverage they want. I'm slammed for the next few weeks, and I only have time to make the updates I suggested above ☝️. If they decide to do more, I request another author take on the task.

@scottaddie
Copy link
Member

I think it's important that we specify the exact SDK version that includes the tool. I say this because the older SDKs are always available for download on the All Downloads page. I can take care of the updates on this one.

@scottaddie scottaddie assigned scottaddie and unassigned guardrex May 23, 2018
@guardrex
Copy link
Collaborator

Thanks @scottaddie ... I'm trying hard now to wrap things up.

@scottaddie scottaddie mentioned this issue May 23, 2018
Merged
scottaddie added a commit that referenced this issue May 24, 2018
@scottaddie
Clarify SDK version containing Secret Manager tool (#6564)
5ffd62e 
Fixes #6523

**Changes**
- Display the **Install the Secret Manager tool** section for all versions of ASP.NET Core, since it's not safe to assume the .NET Core SDK version being used.
- Include the specific SDK version number which includes the Secret Manager tool
- Add note about tool warning

[Internal Review Page](https://review.docs.microsoft.com/en-us/aspnet/core/security/app-secrets?branch=pr-en-us-6564&view=aspnetcore-2.0&tabs=windows#install-the-secret-manager-tool)
@scottaddie
Copy link
Member

@rjamesnw Thank you for your valuable feedback. The doc changes are live now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@scottaddie scottaddie

Labels
Source - Docs.ms Docs Customer feedback via GitHub Issue
Projects
None yet
Milestone
Sprint 136 (5/19/18 - 6/8/18)
Development

Successfully merging a pull request may close this issue.

Add CreateDefaultBuilder content to app secrets dotnet/AspNetCore.Docs
4 participants
@guardrex @rjamesnw @natemcmaster @scottaddie