-
Notifications
You must be signed in to change notification settings - Fork 89
Updated to V4 of the AWS SDK for .NET #393
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.
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the AWS SDK for .NET from version 3.x to version 4.x across all tools in the repository. The update includes migrating from the embedded LitJson library to System.Text.Json and addressing a Docker build issue for Lambda container functions.
- Updated AWS SDK package references from v3.x to v4.x
- Replaced LitJson usage with System.Text.Json throughout the codebase
- Fixed Docker build issues by switching to
docker buildx buildwith--provenance=falseflag
Reviewed Changes
Copilot reviewed 76 out of 76 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Amazon.*.Tools.csproj | Updated AWS SDK package references to v4.x and added language version constraints |
| src/Amazon.Common.DotNetCli.Tools/ExtensionMethods.cs | Added JsonElement extension method for converting to object types |
| src/Amazon.Common.DotNetCli.Tools/DockerCLIWrapper.cs | Modified Docker build to use buildx with provenance flag to fix Lambda deployment issues |
| src/Amazon.Lambda.Tools/LambdaUtilities.cs | Replaced LitJson with System.Text.Json for template parsing |
| test/*.cs | Updated test assertions and removed LitJson dependencies |
Comments suppressed due to low confidence (2)
src/Amazon.Lambda.Tools/LambdaUtilities.cs:1
- This line assigns softLimit value to HardLimit property instead of SoftLimit. This should be
ulimit.SoftLimit = softLimit.GetInt32();
using System;
src/Amazon.ECS.Tools/ECSUtilities.cs:1
- This line assigns softLimit value to HardLimit property instead of SoftLimit. This should be
ulimit.SoftLimit = softLimit.GetInt32();
using Amazon.Common.DotNetCli.Tools;
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| { | ||
| _logger?.WriteLine("The docker CLI \"buildx\" command is used to build ARM64 images. This requires version 20 or later of the docker CLI."); | ||
| arguments.Append($"buildx build --platform linux/arm64 "); | ||
| arguments.Append($"buildx build --platform linux/arm64 --provenance=false "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering why we need this here but not in other places where we build Lambda container images. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where else do we build Lambda Function containers images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our pipelines. Just wondering why that's not an issue.
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFrameworks>netcoreapp3.1;net6.0</TargetFrameworks> | ||
| <TargetFrameworks>net6.0;net8.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sticking with net6 because of the toolkit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't remove .NET 6 because I suspect there are still customers that have gotten on allowed list to still push on .NET 6 functions. Also they could be creating .NET 6 or 7 container image functions and nothing would block them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before merging to dev, don't you want to backup the existing v3 dev and main branches? That way we can hook them up to the V3 pipelines and support them until the end of support date next year?
I don't see a need as we will only support the tip release. If there was an emergency issue we can retroactively create a branch based on the commit. |
… the AWS Toolkit for VS
Description of changes:
Updated to V4 of the AWS SDK for .NET. Besides the usual changes for V4 the tools were also relying on the embedded LitJson copy from AWSSDK.Core. V4 no longer has that embedded JSON parser and updated to System.Text.Json. I had to keep to using JsonElement instead of using the
System.Text.Json.Nodeswith theJsonObjectbecause that namespace is not available in the .NET Standard 2.0 target package which is what is available when the tools are embedded in the VS Toolkit.I also found the container image function tests stopped working. It looks like since our last release docker now defaults to using Buildkit, which is what is be behind
docker buildxand it by default generates an Image Index container. An image index is basically an image with a manifest but Lambda does not support that and the CreateFunction call fails. I had to rework the docker build command to always usedocker buildx buildand pass in the--provenance=falseswitch. Found a related Reddit post of others having this issue although not with these tools. https://www.reddit.com/r/aws/comments/1hf3rrq/struggling_to_deploy_docker_image_to_aws_lambda/By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.