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

Replace all AWS SDK calls with their WithContext analogs #4490

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

Fedosin
Copy link
Contributor

@Fedosin Fedosin commented Sep 7, 2023

What type of PR is this?
/kind support

What this PR does / why we need it:
This is the first PR in a series.

Now we don't pass context from Reconcile functions to AWS SDK calls, which limits our ability to trace requests using OpenTelemetry. To improve this we want to start passing context from top to bottom.

In this PR we just replace AWS SDK calls with their WithContext analogs, so that they can accept a context parameter. In the following PRs we are going to start passing context from Reconcile to the SDK calls.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Partially-Fixes #4504

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

None

@k8s-ci-robot k8s-ci-robot added kind/support Categorizes issue or PR as a support question. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Sep 7, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 7, 2023
Copy link
Member

@Ankitasw Ankitasw left a comment

Choose a reason for hiding this comment

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

We are not passing the context top down still. all the contexts are TODO, so it still not serve the purpose that is stated in the PR

@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 Sep 7, 2023
@Fedosin
Copy link
Contributor Author

Fedosin commented Sep 7, 2023

@Ankitasw yeah, you are right, the description is a bit confusing. I updated it to make it more explicit that in this PR we just replace SDK calls, and all work about passing contexts will be done in the following PRs.

@nrb
Copy link
Contributor

nrb commented Sep 13, 2023

Given that this is the first step in a series of work, replacing the calls with their context-aware equivalents and context.TODO() seems reasonable to me. Skimming the changes, this PR appears to be fairly low risk, though I did not dig into the test changes yet.

@Ankitasw
Copy link
Member

Ankitasw commented Sep 13, 2023

@Fedosin if this is going to be a series of work, can we have a consolidated issue created with the task list?

@Fedosin
Copy link
Contributor Author

Fedosin commented Sep 14, 2023

Thanks folks! I created an issue for this work: #4504

@Ankitasw
Copy link
Member

Ankitasw commented Sep 15, 2023

Thanks @Fedosin , if you have a plan of work, could you mention all the work items in above issue?

@nrb
Copy link
Contributor

nrb commented Sep 21, 2023

Not an official reviewer, but I did another pass focusing on the tests, and this appears to pretty much be straight replacements like I mentioned before.

There are some functions that are expanded to receive requestOptions, but again this PR looks pretty low risk.

LGTM aside from some tracking requests on the issue.

@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 22, 2023
@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 22, 2023
@Ankitasw
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ankitasw

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 26, 2023
@Ankitasw Ankitasw added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 26, 2023
@k8s-ci-robot k8s-ci-robot merged commit 76e369a into kubernetes-sigs:main Sep 26, 2023
8 checks passed
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/support Categorizes issue or PR as a support question. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note-none Denotes a PR that doesn't merit a release note. 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.

Pass context from Reconcile functions to AWS requests
4 participants