Adds the ability to specify an appsettings file for user-jwts #58605

ChrisAnn · 2024-10-24T08:56:27Z

Adds the ability to specify an appsettings file for user-jwts

Description

Currently the user-jwts tool amends appsettings.Development.json when adding a mock JWT scheme for local testing. Many people use appsettings.Development.json for environments other than local development, i.e. a cloud Development environment, and so this amendment causes issues. In my particular case, we have appsettings.Local.json instead and so the tool fails to execute because it cannot find appsettings.Development.json.

As described in #56169 this affects multiple other teams too.

This PR allows for an appsettings file to be specified which will be amended instead of the default appsettings.Development.json. The tool still assumes the file lives next to the project, as it currently does, and checks that the file both exists and ends in .json. Tests have been added to confirm the functionality.

Fixes #56169

Addresses dotnet#56169

ChrisAnn · 2024-11-11T17:06:46Z

@dotnet-policy-service agree company="Stack Overflow"

captainsafia

LGTM overall!

It might be a good idea to move the validation logic that is duplicated across different source files into a single helper to make it easier to keep the validation logic in sync across the different commands.

@halter73 Can you sanity check my review here?

captainsafia · 2024-12-16T18:13:50Z

/azp run

azure-pipelines · 2024-12-16T18:14:10Z

Azure Pipelines successfully started running 3 pipeline(s).

ChrisAnn · 2024-12-17T16:06:09Z

Thanks for taking a look @captainsafia

I've moved the validation logic to reduce duplication as suggested :)

Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.

Files not reviewed (2)

src/Tools/dotnet-user-jwts/src/Resources.resx: Language not supported
src/Tools/dotnet-user-jwts/src/Commands/RemoveCommand.cs: Evaluated as low risk

Comments suppressed due to low confidence (1)

src/Tools/dotnet-user-jwts/src/Helpers/DevJwtCliHelpers.cs:55

The error message 'Resources.RemoveCommand_InvalidAppsettingsFile_Error' is unclear. It should be more descriptive, such as 'The specified appsettings file is invalid. Please provide a valid JSON file.'

reporter.Error(Resources.RemoveCommand_InvalidAppsettingsFile_Error);

captainsafia · 2024-12-17T18:03:37Z

src/Tools/dotnet-user-jwts/test/UserJwtsTests.cs

+
+        var appSettings = JsonSerializer.Deserialize<JsonObject>(File.ReadAllText(appsettings));
+        Assert.Equal("dotnet-user-jwts", appSettings["Authentication"]["Schemes"]["Bearer"]["ValidIssuer"].GetValue<string>());
+        app.Run(["create", "--project", project, "--issuer", "new-issuer", "--appsettings-file", "appsettings.Local.json"]);


The method call should use parentheses instead of square brackets.

Suggested change

app.Run(["create", "--project", project, "--issuer", "new-issuer", "--appsettings-file", "appsettings.Local.json"]);

app.Run(new[] { "create", "--project", project, "--issuer", "new-issuer", "--appsettings-file", "appsettings.Local.json" });

This isn't a good suggestion. We're taking advantage of the new collection expressions language feature here.

captainsafia · 2024-12-17T18:03:38Z

src/Tools/dotnet-user-jwts/src/Helpers/DevJwtCliHelpers.cs

+            }
+            else if (!File.Exists(Path.Combine(Path.GetDirectoryName(projectPath), appsettingsFile)))
+            {
+                reporter.Error(Resources.FormatRemoveCommand_AppsettingsFileNotFound_Error(Path.GetDirectoryName(projectPath)));


The error message 'Resources.FormatRemoveCommand_AppsettingsFileNotFound_Error' should include the filename that was not found to provide more context to the user.

Suggested change

reporter.Error(Resources.FormatRemoveCommand_AppsettingsFileNotFound_Error(Path.GetDirectoryName(projectPath)));

reporter.Error(Resources.FormatRemoveCommand_AppsettingsFileNotFound_Error(Path.Combine(Path.GetDirectoryName(projectPath), appsettingsFile)));

This seems like a good suggestion, especially if the user has provided a non-default value for the upsettings file. @ChrisAnn thoughts?

Seems reasonable to me, along with the suggested wording change copilot made above.
I've pushed the changes :)

captainsafia

LGTM overall!

Copilot provided one helpful suggestion that might be worthwhile to apply.

captainsafia · 2024-12-18T23:48:47Z

/azp run

azure-pipelines · 2024-12-18T23:49:07Z

Azure Pipelines successfully started running 3 pipeline(s).

ChrisAnn · 2024-12-20T17:39:14Z

Apologies for not running the tests locally after changing the error messages 🙈
That should be fixed now 🤞🏻

captainsafia · 2024-12-20T17:51:05Z

Apologies for not running the tests locally after changing the error messages 🙈 That should be fixed now 🤞🏻

🙊 Oops -- I should've caught that before approving and putting this PR on auto-merge. 😬 But hey....hurrah for CI catching human error? I wonder if Copilot would've flagged it. 🤔

dotnet-policy-service · 2024-12-27T17:57:40Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

Adds the ability to specify an appsettings file for user-jwts

baf11a1

Addresses dotnet#56169

dotnet-issue-labeler bot added the area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI label Oct 24, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 24, 2024

mkArtakMSFT assigned adityamandaleeka Oct 24, 2024

dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 31, 2024

adityamandaleeka requested a review from captainsafia November 8, 2024 23:58

adityamandaleeka assigned captainsafia and unassigned adityamandaleeka Nov 8, 2024

captainsafia reviewed Dec 16, 2024

View reviewed changes

captainsafia removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Dec 16, 2024

Move the validation logic into a central location

4af9a4b

captainsafia requested a review from Copilot December 17, 2024 18:01

Copilot AI reviewed Dec 17, 2024

View reviewed changes

captainsafia approved these changes Dec 17, 2024

View reviewed changes

Adds copilot suggestions to improve error messages

26a71e3

captainsafia enabled auto-merge (squash) December 18, 2024 23:49

Fix tests after changing error message when appsettings not found

1f60a9e

auto-merge was automatically disabled December 20, 2024 17:38
Head branch was pushed to by a user without write access

ChrisAnn requested a review from captainsafia December 20, 2024 17:39

dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Dec 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the ability to specify an appsettings file for user-jwts #58605

Adds the ability to specify an appsettings file for user-jwts #58605

ChrisAnn commented Oct 24, 2024

ChrisAnn commented Nov 11, 2024

captainsafia left a comment

captainsafia commented Dec 16, 2024

azure-pipelines bot commented Dec 16, 2024

ChrisAnn commented Dec 17, 2024

Copilot AI left a comment

Copilot AI Dec 17, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

captainsafia Dec 17, 2024

Copilot AI Dec 17, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

captainsafia Dec 17, 2024

ChrisAnn Dec 18, 2024

captainsafia left a comment

captainsafia commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

ChrisAnn commented Dec 20, 2024

captainsafia commented Dec 20, 2024

dotnet-policy-service bot commented Dec 27, 2024

	app.Run(["create", "--project", project, "--issuer", "new-issuer", "--appsettings-file", "appsettings.Local.json"]);
	app.Run(new[] { "create", "--project", project, "--issuer", "new-issuer", "--appsettings-file", "appsettings.Local.json" });

	reporter.Error(Resources.FormatRemoveCommand_AppsettingsFileNotFound_Error(Path.GetDirectoryName(projectPath)));
	reporter.Error(Resources.FormatRemoveCommand_AppsettingsFileNotFound_Error(Path.Combine(Path.GetDirectoryName(projectPath), appsettingsFile)));

Adds the ability to specify an appsettings file for user-jwts #58605

Are you sure you want to change the base?

Adds the ability to specify an appsettings file for user-jwts #58605

Conversation

ChrisAnn commented Oct 24, 2024

Adds the ability to specify an appsettings file for user-jwts

Description

ChrisAnn commented Nov 11, 2024

captainsafia left a comment

Choose a reason for hiding this comment

captainsafia commented Dec 16, 2024

azure-pipelines bot commented Dec 16, 2024

ChrisAnn commented Dec 17, 2024

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Dec 17, 2024

Choose a reason for hiding this comment

captainsafia Dec 17, 2024

Choose a reason for hiding this comment

Copilot AI Dec 17, 2024

Choose a reason for hiding this comment

captainsafia Dec 17, 2024

Choose a reason for hiding this comment

ChrisAnn Dec 18, 2024

Choose a reason for hiding this comment

captainsafia left a comment

Choose a reason for hiding this comment

captainsafia commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

ChrisAnn commented Dec 20, 2024

captainsafia commented Dec 20, 2024

dotnet-policy-service bot commented Dec 27, 2024