Update to .NET 10, adapt code style to Steeltoe conventions#22
Update to .NET 10, adapt code style to Steeltoe conventions#22bart-vmware wants to merge 11 commits into
Conversation
Preview link: https://netcoretoolservice-pr-22.azurewebsites.net/api/new
|
| using Steeltoe.NetCoreToolService.SteeltoeUtils.Diagnostics; | ||
| using Steeltoe.NetCoreToolService.SteeltoeUtils.IO; | ||
|
|
||
| #pragma warning disable S2360 // Optional parameters should not be used |
There was a problem hiding this comment.
There's only one case of this condition in the file, should the exclusion be scoped to that specific instance instead of being file-wide?
|
|
||
| if (nvp[0] == "output") | ||
| { | ||
| output = nvp[1]; |
There was a problem hiding this comment.
While it is a pre-existing condition and probably not too much of a risk, we're using unverified input for the output path here, which could result in unexpected locations for the output. We might want to check for path separators before passing the value along to --output so that we can't end up with a smattering of files in the container that won't get cleaned up.
| throw new CommandException($"'{command}' failed to start; no details available"); | ||
| } | ||
| } | ||
| catch (Exception exception) |
There was a problem hiding this comment.
| catch (Exception exception) | |
| catch (Exception exception) when (e is not CommandException) |
| ArgumentNullException.ThrowIfNull(nuGetId); | ||
|
|
||
| private readonly ILogger<NewController> _logger; | ||
| await _commandExecutor.ExecuteAsync($"{NetCoreTool.Command} new uninstall {nuGetId}", null, -1); |
There was a problem hiding this comment.
Are you sure about using an infinite timeout for all of these dotnet command calls?
|
|
||
| internal static partial class AboutLogger | ||
| { | ||
| [LoggerMessage(LogLevel.Debug, "{Program}, version {Version} [{Commit}]")] |
There was a problem hiding this comment.
I think it would be reasonable to have this continue to log at Info, it should only run once at startup
|
|
||
| // Configure the HTTP request pipeline. | ||
|
|
||
| app.UseAuthorization(); |
There was a problem hiding this comment.
Not to say we shouldn't have auth in this service, but as there's none configured we could just remove this line
| [ | ||
| "\r", | ||
| "\n", | ||
| "\r\n" |
There was a problem hiding this comment.
I don't think the \r\n entry is currently adding any value here... consider reversing the order of the list entries or dropping the last one
| { | ||
| using var buffer = new MemoryStream(); | ||
| using (var archive = new ZipArchive(buffer, ZipArchiveMode.Create, true)) | ||
| ZipArchiveEntry entry = archive.CreateEntry($"{Path.GetRelativePath(rootPath, path)}{Path.DirectorySeparatorChar}"); |
There was a problem hiding this comment.
I know you didn't change this, but the zip spec indicates that only forward slashes are allowed, so what we have now is technically incorrect, at least when running on Windows (but doesn't show up in tests because we look for Path.DirectorySeparatorChar there too)
2 participants
See the commit messages for details.
Important
This requires a change in Azure: add an environment variable
WEBSITES_PORTwith value8080.