Migrate AWS SDK v1 to v2 #4305
Migrate AWS SDK v1 to v2 #4305
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the AWS Go SDK from v1 to v2. The changes are comprehensive, updating dependencies, client initialization, API call signatures, error handling, and various data types to align with the v2 SDK's patterns. The use of context.TODO() is noted as a temporary measure. My review focuses on ensuring the migration is correct and safe. I've identified a couple of critical issues where slice access could lead to a panic if an AWS resource is not found, and I've also provided a suggestion regarding context handling best practices.
mantle/platform/api/aws/ec2.go
Outdated
| input.AllRegions = aws.Bool(true) | ||
| } | ||
| output, err := a.ec2.DescribeRegions(&input) | ||
| output, err := a.ec2.DescribeRegions(context.TODO(), &input) |
There was a problem hiding this comment.
While the PR description acknowledges the use of context.TODO() as a placeholder, it's better practice to use context.Background() for operations that are not part of a specific request's lifecycle. This signals that the operation is intentionally not tied to a request context and is not a forgotten TODO. The long-term solution would be to plumb a context.Context through the public API methods to allow for cancellation and timeouts.
| output, err := a.ec2.DescribeRegions(context.TODO(), &input) | |
| output, err := a.ec2.DescribeRegions(context.Background(), &input) |
|
Thank you so much for looking into this |
No problem! a few steps left but hopefully soon. |
prestist
force-pushed
the
update-aws
branch
3 times, most recently
from
ae76319 to
a023fc8
Compare
|
Ok, CI should pass and this should be RTG. Outstanding question we okay with just "context.background()" for all the Todo's or is that something that should be eval'd at a case by case basis and prob a followup pr? |
There was a problem hiding this comment.
LGTM overall, thank you for working on this! I'm working on testing this by building AWS images using this PR, but the migration looks clean.
Outstanding question we okay with just "context.background()" for all the Todo's or is that something that should be eval'd at a case by case basis and prob a followup pr?
I think it’s fine to switch them all to context.Background() in this PR. That’s what we do in the Azure API code already, so it keeps things consistent across providers. We're not using proper contexts over there either, so If we ever decide to plumb it up with cancellation/timeouts as a whole, that can be its own follow-up PR, but for now using Background() everywhere feels like the right move.
|
I rebased this PR locally onto the current main and ran a round of tests today. I was able to successfully build RHCOS, FCOS, and the WinLI AMIs with no issues. I also ran kola tests in AWS using the FCOS AMI, and all tests passed successfully. 🎉 |
prestist
force-pushed
the
update-aws
branch
from
a023fc8 to
659bfcc
Compare
|
Updated @marmijo should be gtg once ci passes; thank you for the review! |
- Update all imports from aws-sdk-go to aws-sdk-go-v2 - Migrate error handling from awserr.Error to smithy.APIError - Update service client initialization (removed session, added direct clients) - Add context parameters to API calls - Update type references from ec2.Type to ec2types.Type - Add AWS SDK v2 dependencies to go.mod - Update slice handling from []*Type to []Type patterns - Update integer type conversions (int64 to int32) - Update architecture and volume type enums for v2
prestist
force-pushed
the
update-aws
branch
from
b831a5e to
47b9d7d
Compare
fixes:
S1008: should use 'return a.Version == b.Version' instead of 'if a.Version != b.Version { return false }; return true' (gosimple)
Migrate AWS SDK from v1 to v2
No functional changes intended beyond SDK upgrade.
Changes
api.go): replace v1 session withconfig.LoadDefaultConfigand v2 clients.context.Contextto all AWS calls (currentlycontext.TODO()placeholders).awserr.Errortosmithy.APIErrorwitherrors.As().ec2types.*,s3types.*).ArchitectureTypeX8664,PermissionGroupAll,ImageAttributeNameLaunchPermission,SnapshotAttributeNameCreateVolumePermission).[]*Tto[]T/[]stringas required.int32per v2 models (VolumeSize, SG ports).s3types.ObjectCannedACL; v2 uploader/ops with contexts.ec2.NewImageAvailableWaiter(...).Wait(...).TODO:
context.TODO()withcontext.Background()or plumbctx context.Contextthrough public APIs for cancellation/timeouts.