-
-
Notifications
You must be signed in to change notification settings - Fork 74
Migrate to aws sdk go v2 #1070
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
Migrate to aws sdk go v2 #1070
Conversation
|
Hey @johannesvietze, thank you for working on this! |
|
I'm on vacation for two weeks. If there's no hurry, I can have a look after. Otherwise, feel free to take over. |
|
Hi @orlangure, I found some time to fix the test. Could you please check the 4 open TODOs (they are all related to the same thing...)? The aws-sdk-go-v2 migration added a leading slash to some prefixes... |
|
Hey @johannesvietze, I see that TODO comments are only in the test code, so it shouldn't affect the users. The users are free to use whichever aws sdk version they like. The changes behind these comments look good to me. |
sounds good, I have removed the TODOs |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1070 +/- ##
==========================================
- Coverage 85.87% 77.08% -8.79%
==========================================
Files 50 55 +5
Lines 2350 2623 +273
==========================================
+ Hits 2018 2022 +4
- Misses 173 443 +270
+ Partials 159 158 -1 ☔ View full report in Codecov by Sentry. |
|
hey @orlangure could you have a look? not sure why the linter fails... |
29edf33 to
036d9b5
Compare
036d9b5 to
c7bc09c
Compare
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 migrates the codebase from AWS SDK Go v1 to v2 while also making some spelling corrections and minor test adjustments. Key changes include updating AWS SDK instantiations across various packages, revising expected S3 key prefixes in tests, and updating documentation and configuration options.
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| preset/mssql/preset_test.go | Updated container start options and added conditional user setting for CI builds. |
| internal/gnomockd/localstack_test.go | Migrated AWS SDK code and updated S3 key prefix expectations in tests. |
| preset/localstack/options_s3.go | Converted S3 client creation from v1 to v2 with updated error messages. |
| preset/localstack/s3_test.go | Adjusted AWS SDK constructs and test expectations (MaxKeys type and key prefixes). |
| preset/azurite/preset_test.go | Minor formatting adjustments in test output. |
| preset/cockroachdb/preset_test.go | Removed an outdated CockroachDB version from the test suite. |
| preset/localstack/preset_test.go | Updated AWS SDK configuration and SQS/SNS test setups for the new SDK v2. |
| docker.go | Propagated new container user setting from Options to Docker container configuration. |
| options.go | Revised spelling in comments and added a new WithUser option for container user setting. |
| README.md | Updated CockroachDB version support in the documentation. |
Files not reviewed (2)
- go.mod: Language not supported
- internal/gnomockd/testdata/cockroachdb.json: Language not supported
Comments suppressed due to low confidence (4)
internal/gnomockd/localstack_test.go:76
- Verify that the updated expected S3 key prefix '/file-' aligns with the actual key format produced by the AWS SDK v2 implementation.
require.True(t, strings.HasPrefix(*f.Key, "/file-"))
preset/localstack/s3_test.go:63
- Confirm that the new expected prefix '/dir/f-' is correct based on the migration to AWS SDK v2, and that it matches the actual object keys.
require.True(t, strings.HasPrefix(*f.Key, "/dir/f-"))
preset/localstack/s3_test.go:92
- Ensure that all expected S3 key prefixes are consistent after the SDK migration, as the inclusion of a leading '/' may affect test outcomes.
require.True(t, strings.HasPrefix(*f.Key, "/dir/f-"))
options.go:198
- Updated comment spelling from 'behaviour' to 'behavior' for consistency; ensure this change is reflected across the codebase where applicable.
// WithContainerReuse disables Gnomock default behavior of automatic container
c7bc09c to
84781e5
Compare
|
Hey @johannesvietze, it's been a while 😼 Thank you for working on this! |
fixes #1030