Skip to content

✨ Migrate ssm code to AWS SDK v2 #5529

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

Merged
merged 11 commits into from
Jun 28, 2025

Conversation

miyadav
Copy link
Contributor

@miyadav miyadav commented Jun 6, 2025

What type of PR is this?

/kind feature
/kind cleanup

What this PR does / why we need it:

Migrates ssm code to AWS SDK v2
Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #5412

Special notes for your reviewer:

Checklist:

squashed commits
includes documentation
includes emoji in title
adds unit tests
adds or updates e2e tests

Release note:

Migrate ssm code to AWS SDK v2

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 6, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @miyadav. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@miyadav miyadav force-pushed the updateawssdktov2 branch from 3173077 to a6aed6e Compare June 6, 2025 06:41
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 6, 2025
@miyadav miyadav force-pushed the updateawssdktov2 branch 5 times, most recently from acf1453 to 0082989 Compare June 11, 2025 11:06
@miyadav
Copy link
Contributor Author

miyadav commented Jun 11, 2025

/test ?

@k8s-ci-robot
Copy link
Contributor

@miyadav: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@miyadav miyadav changed the title WIP Migrate ssm code to AWS SDK v2 ✨ Migrate ssm code to AWS SDK v2 Jun 11, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2025
@miyadav miyadav force-pushed the updateawssdktov2 branch from 0082989 to 4b07844 Compare June 12, 2025 04:34
@punkwalker
Copy link
Contributor

@miyadav The client implementation does not look correct. Please refer to #5497 or #5498 for examples.

Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on! I've left a few comments. As Punkaj suggested, review the PRs that have merged before and try to follow their structure.

Clients should go in pkg/cloud/scope/clients.go and the interfaces should go in pkg/cloud/services/<service name>/service.go.

@miyadav miyadav force-pushed the updateawssdktov2 branch 2 times, most recently from a5f6060 to 124624c Compare June 13, 2025 06:21
@miyadav
Copy link
Contributor Author

miyadav commented Jun 13, 2025

updated , thanks @punkwalker , @nrb .

@miyadav miyadav force-pushed the updateawssdktov2 branch from 124624c to 6d18363 Compare June 13, 2025 07:42
Comment on lines +56 to +74
func isErrorRetryable(err error, retryableCodes []string) bool {
// Use the ParseSmithyError utility to parse the error
smithyErr := awserrors.ParseSmithyError(err)
if smithyErr == nil {
return false
}

// Get the error code from the parsed error
codeToCheck := smithyErr.ErrorCode()

// Compare the extracted string with your list
for _, code := range retryableCodes {
if codeToCheck == code {
return true // It's a match!
}
}
return false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @punkwalker , but , I am using ParseSmithyError from the awserrors package in the isErrorRetryable function, my intent was to rely on the centralized logic in ParseSmithyError , let me know if I have misunderstood , your comment.

@miyadav miyadav force-pushed the updateawssdktov2 branch from a01df82 to 9b0502a Compare June 26, 2025 04:05
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2025
@miyadav miyadav force-pushed the updateawssdktov2 branch from 9b0502a to ab9a661 Compare June 26, 2025 04:31
@miyadav
Copy link
Contributor Author

miyadav commented Jun 26, 2025

/test pull-cluster-api-provider-aws-e2e-blocking

@damdo
Copy link
Member

damdo commented Jun 26, 2025

/test pull-cluster-api-provider-aws-build-docker

@damdo damdo requested a review from punkwalker June 26, 2025 12:31
Copy link
Contributor

@punkwalker punkwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@damdo
Copy link
Member

damdo commented Jun 27, 2025

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

@damdo
Copy link
Member

damdo commented Jun 27, 2025

/test pull-cluster-api-provider-aws-e2e

Copy link
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a LOT @miyadav 🎉

All tests passed and approvers/reviewers are happy.

/lgtm

/unhold

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 27, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 30f2438df768fe37b39adcfe226249799ec0a67c

@damdo
Copy link
Member

damdo commented Jun 27, 2025

/test pull-cluster-api-provider-aws-build-docker

4 similar comments
@damdo
Copy link
Member

damdo commented Jun 27, 2025

/test pull-cluster-api-provider-aws-build-docker

@damdo
Copy link
Member

damdo commented Jun 27, 2025

/test pull-cluster-api-provider-aws-build-docker

@damdo
Copy link
Member

damdo commented Jun 28, 2025

/test pull-cluster-api-provider-aws-build-docker

@damdo
Copy link
Member

damdo commented Jun 28, 2025

/test pull-cluster-api-provider-aws-build-docker

@k8s-ci-robot k8s-ci-robot merged commit 4b2f770 into kubernetes-sigs:main Jun 28, 2025
19 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v2.8 milestone Jun 28, 2025
@AndiDog AndiDog mentioned this pull request Jul 9, 2025
5 tasks
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate ssm code to AWS SDK v2
6 participants