-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
chore: migrate s3 ops to sdk v2, add workflow for loading sdk v2 config #376
base: main
Are you sure you want to change the base?
Conversation
@lucix-aws This is amazing. It was on my list to look at doing this in earnest this winter, but this looks like a great start. It has been a while since I've run the integration tests, so it's possible they are broken, I will take a look. Generally not worried about exported APIs in this project, it's not meant to be directly used, and if it is it's usually at a higher level that making changes to the individual resources shouldn't be a problem. I hadn't yet decided if I was going to run them in parallel or not. Ultimately I think I might have to as it's quite the lift to shift everything over at once especially with so many changes. I will definitely take a closer look at this and this will help me form the strategy for migrating. |
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.
nice example to learn migration in detail, left some minor comments
This fix fixes the integration tests, you'll likely need to rebase. |
ac8b149
to
1f3aa57
Compare
Patch addresses PR comments, lint, and rebases for the integration tests. |
I PR'd the addition of |
Upgrading s3 to using the sdk v2 could also be handy for using list buckets. It's now possible to filter list buckets on a region: https://github.com/aws/aws-sdk-go-v2/releases/tag/release-2024-10-16 |
We are mid-hackathon in SDKs and Tools right now, will get back to this next week. |
No problem @lucix-aws. Looking forward to when this can be merged and we can the ball rolling on moving to sdk v2 for all resources. |
1f3aa57
to
01608c6
Compare
Fixed commits and pushed an update to use the new |
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.
Looks great @lucix-aws I really appreciate this, going to save me a lot of time. Looks like we have a small merge conflict on go.mod though. I'll start testing it locally in the mean time.
01608c6
to
9c45415
Compare
Part of #187
I identified this project as an advanced user of the Go SDK for the following reasons:
I did this migration as an exercise to get a better idea for what doing so is like in a large project and to see if there's anything we can improve in that area. This made me realize that an outstanding issue we have aws/aws-sdk-go-v2#2422 is more important of a parity gap than I thought, its priority has been increased accordingly.
What this PR does:
aws.Config
, which is the v2 equivalent ofsession.Session
.ctx
everywhere for doing most/all service calls, butconfig.LoadDefaultConfig
also takes one, in places where I didn't have access I just passedcontext.TODO()
What I tested:
You definitely can have both SDK v1 and v2 live alongside each other, but you may or may not wish to cut releases in that transition period. If that's the case I would just change this to merge to a feature branch.