Add port environment variables for Aspire integration #1942
Conversation
| @@ -107,7 +107,7 @@ public static (IDictionary<string, string>, IDictionary<string, IList<string>>) | |||
| { | |||
| var key = lowerCaseKeyName ? header.Key.ToLower() : header.Key; | |||
| singleValueHeaders[key] = header.Value.Last() ?? ""; | |||
| multiValueHeaders[key] = [.. header.Value]; | |||
| multiValueHeaders[key] = [.. header.Value!]; | |||
There was a problem hiding this comment.
I think the null warnings happening in this file were a false positive. They warnings only show up when doing dotnet pack not in VS. And the Value is a struct so it can't be null.
| var encodedPathParameters = pathParameters.ToDictionary( | ||
| kvp => kvp.Key, | ||
| kvp => Uri.EscapeUriString(kvp.Value)); // intentionally using EscapeURiString over EscapeDataString since EscapeURiString correctly handles reserved characters :/?#[]@!$&'()*+,;= in this case | ||
| kvp => Uri.EscapeUriString(kvp.Value)); // intentionally using EscapeUriString over EscapeDataString since EscapeUriString correctly handles reserved characters :/?#[]@!$&'()*+,;= in this case |
There was a problem hiding this comment.
Is there no better way for us to have the same behavior without using an obsolete method? What if they drop it in .NET10?
There was a problem hiding this comment.
@gcbeattyAWS Can you answer this question since this was your change?
There was a problem hiding this comment.
so i tried looking for an alternative function that was not deprecated and produced the same output that we needed - but i could not find any. i think i tried 5 or 6 different ways and spent a couple hours on it. If we want to ensure the function never gets dropped I think we could just re-implement/copy their source code for it no?
There was a problem hiding this comment.
I'm not worried about the method being dropped. Methods being dropped from the BCL is a big deal that is rarely done.
| @@ -19,4 +19,17 @@ | |||
| config.SetApplicationName(Constants.ToolName); | |||
| }); | |||
|
|
|||
| return await app.RunAsync(args); | |||
| var arguments = new List<string>(args); | |||
There was a problem hiding this comment.
I prefer that you not modify the arguments directly and instead add your logic to https://github.com/aws/aws-lambda-dotnet/blob/feature/lambdatesttool-v2/Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Commands/RunCommand.cs or https://github.com/aws/aws-lambda-dotnet/blob/feature/lambdatesttool-v2/Tools/LambdaTestTool-v2/src/Amazon.Lambda.TestTool/Commands/Settings/RunCommandSettings.cs
Description of changes:
In Aspire we don't know the ports that it will allocate for the test tool till after the command line parameters have been set for the executable. To work around this I added environment variables that can be set by Aspire for the executable to pick up and use for ports.
I had trouble loading the test tool project for debugging due to the recent commit adding the wwwroot folder for packaging. To get around that I have the property only be set in Release mode allowing the project to be loaded in visual studio and
dotnet packwhich is run in release mode to find the static content. Not a perfect solution because it means VS will have issues in release mode. Unblocks for now but open to better solution.I also addressed the remaining warnings in the project.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.