Skip to content
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 #4640

Merged
merged 4 commits into from
Sep 7, 2024
Merged

Conversation

mjlshen
Copy link
Contributor

@mjlshen mjlshen commented Jul 29, 2024

Description
This PR migrates the codebase to aws-sdk-go-v2 by migrating these three main components:

  • The DynamoDB registry functionality
  • The AWS-SD (Cloud Map) provider
  • The AWS (Route53) provider

Which then allowed me to:

My commits are all self-contained to align with those 4 steps, so I'd be happy to split this PR up if that would be preferred.

Fixes #4637

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 29, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 29, 2024
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 30, 2024
@mjlshen mjlshen changed the title Migrate DynamoDB registry to aws-sdk-go-v2 Migrate to aws-sdk-go-v2 Jul 30, 2024
@mjlshen mjlshen force-pushed the aws-sdk-go-v2 branch 3 times, most recently from 0406eb8 to 4dc5c92 Compare July 30, 2024 04:18
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2024
@mloiseleur
Copy link
Contributor

Thanks for this @mjlshen. It's amazing.
I've taken a look at the test, the code and you added error check when possible.
Considering the major impact of this PR, it would be nice if some AWS user can test it for real.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2024
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 9, 2024
@mjlshen
Copy link
Contributor Author

mjlshen commented Aug 9, 2024

Thanks @mloiseleur! I needed to rebase to resolve conflicts in go.mod - I'm happy to help test this on AWS. I'll plan on going through https://kubernetes-sigs.github.io/external-dns/v0.14.2/tutorials/aws-sd/ and https://kubernetes-sigs.github.io/external-dns/v0.14.2/tutorials/aws/ to make sure these still work and will report back with how it went. If there's anything else I can help with please feel free to let me know as well.

  • Validate AWS Cloud Map API Docs
  • Validate AWS (Route53) Docs
  • Validate DynamoDB Registry

provider/aws/aws.go Outdated Show resolved Hide resolved
@szuecs
Copy link
Contributor

szuecs commented Aug 12, 2024

@mjlshen I am not very confident with this change because it has too many changes in one. This one ea92169 is the commit that worries me, rest seems fine. The logic in the commit changes way too much for a refactoring that should focus on limiting the logic change if having even one.

@mjlshen
Copy link
Contributor Author

mjlshen commented Aug 17, 2024

I'm happy to help test this on AWS. I'll plan on going through https://kubernetes-sigs.github.io/external-dns/v0.14.2/tutorials/aws-sd/ and https://kubernetes-sigs.github.io/external-dns/v0.14.2/tutorials/aws/ to make sure these still work and will report back with how it went. If there's anything else I can help with please feel free to let me know as well.

@szuecs / @mloiseleur I have (finally :D) validated my code in AWS by:

  • Running through the AWS Cloud Map Docs (Ensure ability to create/delete Route53 Records via Cloud Map with the TXT registry)
  • Running through the AWS (Route53) Docs (Ensure ability do create/delete Route53 Records via a public hosted zone with the TXT registry)
  • Setting up a DynamoDB registry and running through the AWS (Route53) Docs (Ensure ability to create/delete Route53 Records via a public hosted zone with the DynamoDB registry)

So I'm confident that it works! Is there anything else I can help test or demonstrate?

provider/zone_type_filter_test.go Show resolved Hide resolved
provider/aws/aws.go Show resolved Hide resolved
@mjlshen mjlshen requested a review from szuecs August 31, 2024 21:09
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2024
Signed-off-by: Michael Shen <mishen@umich.edu>
Signed-off-by: Michael Shen <mishen@umich.edu>
Signed-off-by: Michael Shen <mishen@umich.edu>
Signed-off-by: Michael Shen <mishen@umich.edu>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2024
@szuecs
Copy link
Contributor

szuecs commented Sep 6, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: szuecs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2024
@mjlshen
Copy link
Contributor Author

mjlshen commented Sep 6, 2024

@mloiseleur when you have time, it would be great to have a re-review on this one. Thanks!

@mloiseleur
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2024
@k8s-ci-robot k8s-ci-robot merged commit 4333b31 into kubernetes-sigs:master Sep 7, 2024
13 checks passed
@mjlshen mjlshen deleted the aws-sdk-go-v2 branch September 7, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

switch to aws-sdk-go-v2 as v1 is going away
4 participants