-
Notifications
You must be signed in to change notification settings - Fork 623
✨ Migrate Node packages to AWS SDK v2 #5584
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @LiangquanLi930! |
Hi @LiangquanLi930. 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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
@damdo Hi, I have a question I'd like to ask — regarding cluster-api-provider-aws/pkg/cloud/interfaces.go Lines 33 to 38 in 701052c
as part of this issue we’re trying to address? |
Hey @LiangquanLi930 I don't think so, but better to ask @punkwalker |
I saw #5521 — maybe we should wait for this PR to merge before proceeding with ours. |
Yeah, that's probably better, let's pause this until we get the other in, thanks @LiangquanLi930 |
@LiangquanLi930 @damdo The session() can only be removed if all the dependent clients are migrated to SDK V2. Let’s wait for #5521 to merge and then cleanup every reference to SDK v1 sessions. |
462b877
to
4c85641
Compare
@LiangquanLi930 looks like there is a compilation error |
4c85641
to
1e22f5e
Compare
Still present it looks like @LiangquanLi930 |
I am taking it now |
@LiangquanLi930 other PRs seems to be now fine with https://prow.k8s.io/job-history/gs/kubernetes-ci-logs/pr-logs/directory/pull-cluster-api-provider-aws-e2e-blocking as you can see. Anything that could be causing that issue in this PR? |
949dffd
to
8d888da
Compare
cd2370f
to
191a56f
Compare
/test pull-cluster-api-provider-aws-test |
1 similar comment
/test pull-cluster-api-provider-aws-test |
c89054e
to
89043d1
Compare
89043d1
to
9c4c8f4
Compare
@LiangquanLi930: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
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.
Thanks a lot @LiangquanLi930
@punkwalker would you be able to take a look and confirm this looks in line with what you expect to see? Thanks!
/assign @nrb for review |
/test pull-cluster-api-provider-aws-e2e-eks pull-cluster-api-provider-aws-e2e |
/label tide/merge-method-squash I'll set this to squash automatically, but in the future, try to squash changes into a single commit, especially when removing debugging commits. |
@@ -43,9 +44,9 @@ const ( | |||
|
|||
var retryableErrors = []string{ | |||
// Returned when the secret is scheduled for deletion | |||
secretsmanager.ErrCodeInvalidRequestException, | |||
"InvalidRequestException", |
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.
Is there really no replacement constant in SDK v2?
@@ -158,6 +158,10 @@ func Node1BeforeSuite(e2eCtx *E2EContext) []byte { | |||
e2eCtx.Environment.BootstrapAccessKey = newUserAccessKey(context.TODO(), e2eCtx.AWSSessionV2, bootstrapTemplate.Spec.BootstrapUser.UserName) | |||
e2eCtx.BootstrapUserAWSSession = NewAWSSessionWithKey(e2eCtx.Environment.BootstrapAccessKey) | |||
e2eCtx.BootstrapUserAWSSessionV2 = NewAWSSessionWithKeyV2(e2eCtx.Environment.BootstrapAccessKey) | |||
|
|||
By("Waiting for access key to propagate...") | |||
time.Sleep(10 * time.Second) |
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.
Is there a condition that we can use an Eventually
on, versus a static 10s wait?
What type of PR is this?
Add one of the following kinds:
/kind feature
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #5402
Special notes for your reviewer:
Checklist:
Release note: