-
Notifications
You must be signed in to change notification settings - Fork 0
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
AWS SDK Go V2 Migration #11
Conversation
Overall, looks good to me, once the other comments have been addressed. |
@@ -30,7 +30,8 @@ type configuration struct { | |||
var ( | |||
enableLogConfig = &configuration{flag: "enable-logging", envFlag: "enable_logging", defaultValue: "true"} | |||
regionConfig = &configuration{flag: "region", envFlag: "region", defaultValue: "us-east-1"} | |||
maxRetriesConfig = &configuration{flag: "max-retries", envFlag: "max_retries", defaultValue: strconv.Itoa(retry.DefaultMaxAttempts)} | |||
maxReadRetriesConfig = &configuration{flag: "max-read-retries", envFlag: "max_read_retries", defaultValue: strconv.Itoa(retry.DefaultMaxAttempts)} | |||
maxWriteRetriesConfig = &configuration{flag: "max-write-retries", envFlag: "max_write_retries", defaultValue: strconv.Itoa(10)} |
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.
keeping this to 10 from before, also helps distinguish in tests
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 good!
- Unit tests passed.
- Integration tests passed.
- TLS tests passed.
- Correctness tests passed.
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 good. I validated all tests are passing and deployed the connector to test ingestion and remote_read. Can you add a note in the PR description about the returned status code changes for badRequest
and statusConflict
.
Issue #, if available:
Description of changes:
This change upgrades the underlying AWS Go SDK from V1 to V2.
Some notes and highlights regarding significant changes in V2 for your reference:
config.LoadDefaultConfig()
) which consolidates loading AWS credentials, regions, etcaws.String()
wrappers and providing typed fields where possible)context.Context
(supporting graceful cancellation, timeouts etc)Smithy
middleware. Errors now follow a more consistent structure influenced by Smithy’s model-driven error definitions, making it easier to identify and handle specific error conditions (likeResourceNotFound
orAccessDenied
) through typed errors rather than relying on string matching.HasMorePages()
andNextPage()
) to handle multi-page API responses. In our context,NextPage
performs the network call for fetching query responses (previouslyQueryPages
).goimports
, let me know if you guys have a particular preference or style.docker-compose.yml
as I find it useful, but I can remove it if it clutters the repo.Additional resources:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.