go-cloud icon indicating copy to clipboard operation
go-cloud copied to clipboard

Docstore/awsdynamodb: Supporting the AWS V2 SDK

Open Maldris opened this issue 10 months ago • 11 comments

Resolved #3458

A somewhat quick and dirty conversion to using the AWS V2 SDK rather than the deprecated V1.

Due to the delays in my finding time to work on this I prioritised getting the functionality migrated to mitigate any risk from API drift form the deprecated V1 SDK.

The main sections that warrant further discussion:

  • codec behaviour could be handled better with type switches now, but may be a follow up task
  • url opener doesn't use a configuration override any more, most of this is handled by V2ConfigFromURLParams, but is worth review
  • error handling using an As pattern and I've declared all the as targets in a block which is likely less memory efficient, but simple to read, should another approach be used?

Maldris avatar Jan 31 '25 06:01 Maldris

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Jan 31 '25 06:01 google-cla[bot]

Hi Johnathan,

Last year the AWS SDK V1 was deprecated (see announcement about maintenance and end of support in project readme) with its maintenance grace period set to end on 7/31/2025, this PR was created to address that and move the package to using the new SDK as all the other modules already have.

This was discussed in the flagged issue #3458 Forms part of the larger body of work supporting #3489 And follows all the other modules being moved to use V2 as the default (#3465 and #3466)

docstore/awsdynamodb was the only module without a V2 implementation, and as seen in #3458 was discussed as being better served as a breaking replacement ahead of removing V1 from other modules, rather than the more complicated task of building a complete abstraction of the behaviour to support both, or having duplicated code for both.

I'll be adding comments on the three sections you've specifically flagged as well.

Maldris avatar Feb 03 '25 02:02 Maldris

Apologies for the mentions but as its been a week and I've been seeing notification issues in other repo's this week; so just to be sure @jba and because the prior conversation about this was with you, @vangent

Maldris avatar Feb 07 '25 07:02 Maldris

@Maldris, I read #3458 as concluding that a v2 fork was the right approach. I'm in favor of that.

jba avatar Feb 07 '25 18:02 jba

ok, did a little history re-writing to move things into docstore/awsdynamodbv2 while preserving change history a bit better (though it forced changing all the commit dates to today to keep it simpler)

I still have the other variant on a backup branch locally, but does this pass requirements better @jba ?

Maldris avatar Feb 11 '25 00:02 Maldris

Can we make the path docstore/awsdynamodb/v2? That's typically what we do. For example, math/rand and math/rand/v2.

jba avatar Mar 10 '25 13:03 jba

Can you merge this with HEAD and push it forward? It's the only thing blocking removal of V1 entirely.

vangent avatar Mar 31 '25 04:03 vangent

merged and updated with the changes to the docstore test harness added in V1 that needed to be replicated in V2.

Maldris avatar Mar 31 '25 04:03 Maldris

@Maldris looks like we still have some failing checks.

jba avatar Mar 31 '25 18:03 jba

@Maldris @jba Do we have any update for this PR? aws-sdk-go will be end of support in < 2months.

youngbupark avatar Jun 03 '25 14:06 youngbupark

Fix tests, then we can submit.

jba avatar Jun 08 '25 11:06 jba

Try syncing to head and re-push to see if tests pass.

vangent avatar Jun 28 '25 19:06 vangent

I cloned this and made some fixes, see https://github.com/google/go-cloud/pull/3602.

vangent avatar Jul 19 '25 01:07 vangent

apologies, I've been dealing with some medical and family issues and this got lost in the noise till these notifications.

Thank you for finalizing the updates, I'll hopefully have a chance to use it once I'm back at work

Maldris avatar Jul 20 '25 10:07 Maldris

No worries, and thanks for the contribution! Hope things are solid enough you can get back to work soon.

vangent avatar Jul 20 '25 16:07 vangent