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.

Sign up for GitHub

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

Jump to bottom

⚠️ Propagate context.Context throughout the codebase #1116

Merged
merged 1 commit into from Oct 1, 2020
Merged

⚠️ Propagate context.Context throughout the codebase #1116

merged 1 commit into from Oct 1, 2020

Conversation

vincepri
Copy link
Member

@vincepri vincepri commented Aug 5, 2020

Signed-off-by: Vince Prignano vincepri@vmware.com

/assign @detiber @alvaroaleman
/milestone v0.7.x

If anyone has better ideas on how to propagate this context, please let me know 😄

@k8s-ci-robot k8s-ci-robot added this to the v0.7.x milestone Aug 5, 2020
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 5, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2020
alvaroaleman
alvaroaleman approved these changes Aug 5, 2020
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

So @detiber can confirm

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 5, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2020
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2020
@vincepri vincepri changed the title 🌱 Propagate context from controller.Start to handler [WIP] 🌱 Propagate context from controller.Start to handler Aug 5, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2020
@vincepri vincepri force-pushed the propagate-context-worker branch 2 times, most recently from 5fb5c65 to 5ad43b9 Compare August 6, 2020 14:09
@vincepri vincepri changed the title [WIP] 🌱 Propagate context from controller.Start to handler [WIP] ⚠️ Propagate context.Context throughout the codebase Aug 6, 2020
vincepri
vincepri commented Aug 6, 2020
View reviewed changes
pkg/manager/internal.go Show resolved Hide resolved
pkg/manager/internal.go Show resolved Hide resolved
pkg/manager/internal.go Show resolved Hide resolved
@vincepri vincepri changed the title [WIP] ⚠️ Propagate context.Context throughout the codebase ⚠️ Propagate context.Context throughout the codebase Aug 6, 2020
@vincepri vincepri force-pushed the propagate-context-worker branch 5 times, most recently from 42e8e9c to a33241c Compare September 22, 2020 14:59
@vincepri
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2020
@vincepri vincepri force-pushed the propagate-context-worker branch 3 times, most recently from 00d6b3b to 3a16b41 Compare September 23, 2020 13:31
alvaroaleman
alvaroaleman reviewed Sep 28, 2020
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

Two questions, other than that lgtm. It would be great if we could fine someone else to give this a pass though in case I missed something due to the sheer size of the PR.

pkg/cache/cache_test.go Outdated Show resolved Hide resolved
pkg/manager/manager_test.go Show resolved Hide resolved
@vincepri vincepri force-pushed the propagate-context-worker branch 2 times, most recently from ad5cd1c to 7131cd8 Compare September 28, 2020 20:45
@vincepri
Copy link
Member Author

@DirectXMan12 @ncdc @detiber If you have some time, an extra set of 👀 would be greatly appreciated

alvaroaleman
alvaroaleman approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold
so others can have a look

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

ncdc commented Sep 29, 2020

I think this LGTM. There's an open question still for FakeInformers.Start() - ?

@vincepri
Copy link
Member Author

vincepri commented Sep 29, 2020 •
edited

Thanks! I'm ok having that come in later, it might break tests (and users) if we return ctx.Err(), which seems we've decided not to do in #1116 (comment)

@vincepri
Copy link
Member Author

vincepri commented Oct 1, 2020

/hold cancel

Checked with @alvaroaleman before removing the hold, let's get this merged so we can unblock other work :)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2020
@vincepri
⚠️ Propagate context.Context throughout the codebase
8d45507 
Signed-off-by: Vince Prignano <vincepri@vmware.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 1, 2020 •
edited

@vincepri: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-controller-runtime-apidiff-master 8d45507 link /test pull-controller-runtime-apidiff-master

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/test-infra repository. I understand the commands that are listed here.

@alvaroaleman
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2020
@k8s-ci-robot k8s-ci-robot merged commit 7f050c2 into kubernetes-sigs:master Oct 1, 2020
@alvaroaleman alvaroaleman mentioned this pull request Oct 1, 2020
Closed
@timebertt timebertt mentioned this pull request Dec 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DirectXMan12 DirectXMan12 DirectXMan12 left review comments

@joelanford joelanford joelanford left review comments

@detiber detiber detiber left review comments

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@alexeldeib alexeldeib Awaiting requested review from alexeldeib

@estroz estroz Awaiting requested review from estroz

Assignees

@detiber detiber

@alvaroaleman alvaroaleman

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v0.7.x
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@vincepri @k8s-ci-robot @alvaroaleman @ncdc @DirectXMan12 @joelanford @detiber