Add an init command for adding a UserSecretsId #500

liamdawson · 2018-10-13T04:04:17Z

Adds an init command for adding a UserSecretsId

Addresses #166

dnfclas · 2018-10-13T04:04:28Z

All CLA requirements met.

liamdawson · 2018-10-13T04:07:29Z

There are a couple of things I would like some help fixing appropriately:

Any test cases I'm missing? e.g. I feel I am missing test cases for invalid/missing project files for init, as they won't return the same output as they do for other commands right now.
As the above point, some error cases (invalid/missing project file) won't return output consistent with other subcommands - what's the best DRY way to solve that?
How should I indicate a command failure for https://github.com/aspnet/DotNetTools/pull/500/files#diff-0207ee8b1aa083ec78304322c8b620c7R65 ?
How do I add a localized error for "Project already has a UserSecretsId"? Adding it to the resources.resx didn't add the functions in the designer.cs that exist for the other resource entries.

natemcmaster · 2018-10-15T18:40:56Z

Hi @liamdawson, thanks for doing this work! I'm going to help you merge this PR in. Unfortunately, we've missed the deadline for features in 2.2, so this PR will need to target 3.0 instead. I'm going to re-target your PR to the master branch and then review the code changes.

Thanks,
Nate

natemcmaster

This looks like a great start! I've left a few comments about specific parts of this implementation.

Can you also add test case for this particular error case?

Init should failed when a bad ID given. Example: init --id id/with/slash should fail.

And ID is invalid if userSecretsId.IndexOfAny(Path.GetInvalidFileNameChars()) >= 0

src/dotnet-user-secrets/Internal/InitCommand.cs

natemcmaster · 2018-10-15T18:48:20Z

src/dotnet-user-secrets/Internal/InitCommand.cs

@@ -0,0 +1,99 @@
+using Microsoft.Extensions.CommandLineUtils;


Two code formatting nit-picks and all new *.cs files:

Can you add the copyright header?

Can you sort using with System.* namespaces first? I wish VS enforced this better... :-/

DotNetTools/.editorconfig

Lines 12 to 14 in 581b316

[*.cs]

indent_size = 4

dotnet_sort_system_directives_first = true:warning

natemcmaster · 2018-10-15T18:49:54Z

src/dotnet-user-secrets/Internal/InitCommand.cs

+            // No valid property group, create a new one
+            if (propertyGroup == null)
+            {
+                propertyGroup = XElement.Parse(@"<PropertyGroup></PropertyGroup>");


Does this work? Parsing seems like a little much.

propertyGroup = new XElement("PropertyGroup");

Right, I didn't actually try that because I was a bit superstitious about XName. Will take a look later.

natemcmaster · 2018-10-15T18:50:37Z

src/dotnet-user-secrets/Internal/InitCommand.cs

+            }
+
+            // Add UserSecretsId element
+            var userSecretsElement = XElement.Parse(@"<UserSecretsId></UserSecretsId>");


Does this work?

var userSecretsElement = new XElement("UserSecretsId", newSecretsId);

natemcmaster · 2018-10-15T18:51:43Z

src/dotnet-user-secrets/Program.cs

+            if (options.Command is InitCommand initCmd)
+            {
+                initCmd.Execute(new CommandContext(null, reporter, _console), _workingDirectory);
+                return 0;


In retrospec, Command.Execute should probably have returned int so the command can set an exit code. I'd be happy to also take a PR that made this change.

Okay - I looked at that but ran into the fact the interface had the expectation. At first glance though, I don't know how I'd safely modify the interface to allow a status code return without causing a breaking change (but that's a separate conversation).

You have my blessing to make a breaking change on ICommand. For projects like this, the ICommand type is an internal implementation detail of these console tools. No one should be compiling an app against dotnet-user-secrets.dll.

The ICommand in this instance is from CommandLineUtils, so not part of this tool - do you still want that change?

liamdawson · 2018-10-15T20:15:08Z

Thanks for the thorough feedback @natemcmaster, I'll see to that soon.

Could I also get a pointer on how to add messages via resources.resx? I'll need to add a couple, and adding them in VS didn't seem to update the appropriate designer file (which says it is auto-generated).

Thanks again!

natemcmaster · 2018-10-15T20:39:52Z

Re: resx files.

Edit the .resx file in VS (or an xml editor) and then run dotnet msbuild -restore -t:Resx from command line.

natemcmaster · 2018-10-15T20:40:56Z

Sorry, missed a piece: run dotnet msbuild -restore -t:Resx in the src/dotnet-user-secrets/ project dir.

As per #500 (comment)

liamdawson · 2018-10-28T03:48:30Z

I'll open another PR to cover the status code returns on ICommand, otherwise I believe I've addressed all your PR feedback (and have removed changes to korebuild and csproj files)

natemcmaster · 2018-10-29T20:39:17Z

@liamdawson ACK saw your comments. I'll take a closer look at this in a few days. Currently working on time-sensitive issues. Please ping me on this PR if I don't surface for air by Thurs. Nov. 1.

liamdawson · 2018-11-03T04:28:02Z

@natemcmaster, belatedly pinging you as requested.

natemcmaster · 2018-11-05T18:38:22Z

Thanks for making the requested changes. I'll bring this in once CI is green (I've just re-triggered a build).

liamdawson added 5 commits October 13, 2018 13:24

add init call tests

cbb8e81

add init command triggering secrets id error case

33cbf5e

fix init case to support cases without secrets id

24e0cea

correctly resolve project file path

3c97868

add basically functional init command

f3b2203

natemcmaster changed the base branch from release/2.2 to master October 15, 2018 18:41

natemcmaster suggested changes Oct 15, 2018

View reviewed changes

natemcmaster self-assigned this Oct 15, 2018

natemcmaster added this to the 3.0.0 milestone Oct 15, 2018

liamdawson added 4 commits October 28, 2018 13:47

Update init invocation logic

197039e

As per #500 (comment)

add invalid ID init test

ac6f992

fix invalid ID case, use resources for strings

76c86ac

fix bug preventing id and project passing to init

5cb03dd

liamdawson changed the title ~~[WIP] Add an init command for adding a UserSecretsId~~ Add an init command for adding a UserSecretsId Oct 28, 2018

Merge remote-tracking branch 'upstream/master' into add-init-command

d14d917

natemcmaster approved these changes Nov 5, 2018

View reviewed changes

natemcmaster merged commit 9de0452 into aspnet:master Nov 5, 2018

natemcmaster mentioned this pull request Nov 5, 2018

Add user-secret init command #166

Closed

Witik mentioned this pull request Jul 13, 2021

Initialize user secrets does not work with older dotnet user-secrets Witik/RiderUserSecrets#21

Closed

		@@ -0,0 +1,99 @@
		using Microsoft.Extensions.CommandLineUtils;

	[*.cs]
	indent_size = 4
	dotnet_sort_system_directives_first = true:warning

Add an init command for adding a UserSecretsId #500

Add an init command for adding a UserSecretsId #500

Uh oh!

Conversation

liamdawson commented Oct 13, 2018

Uh oh!

dnfclas commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liamdawson commented Oct 13, 2018

Uh oh!

natemcmaster commented Oct 15, 2018

Uh oh!

natemcmaster left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamdawson commented Oct 15, 2018

Uh oh!

natemcmaster commented Oct 15, 2018

Uh oh!

natemcmaster commented Oct 15, 2018

Uh oh!

liamdawson commented Oct 28, 2018

Uh oh!

natemcmaster commented Oct 29, 2018

Uh oh!

liamdawson commented Nov 3, 2018

Uh oh!

natemcmaster commented Nov 5, 2018

Uh oh!

Uh oh!

dnfclas commented Oct 13, 2018 •

edited

Loading