Skip to content

⚠️ Propagate context.Context throughout the codebase#1116

Merged
k8s-ci-robot merged 1 commit intokubernetes-sigs:masterfrom
vincepri:propagate-context-worker
Oct 1, 2020
Merged

⚠️ Propagate context.Context throughout the codebase#1116
k8s-ci-robot merged 1 commit intokubernetes-sigs:masterfrom
vincepri:propagate-context-worker

Conversation

@vincepri
Copy link
Copy Markdown
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
Copy Markdown
Contributor

k8s-ci-robot commented Aug 5, 2020

[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

Details 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
@vincepri vincepri force-pushed the propagate-context-worker branch from 8bea643 to efb3ed1 Compare August 5, 2020 16:10
alvaroaleman
alvaroaleman reviewed Aug 5, 2020
View reviewed changes
@vincepri vincepri force-pushed the propagate-context-worker branch from efb3ed1 to 09dd8cc Compare August 5, 2020 16:20
alvaroaleman
alvaroaleman approved these changes Aug 5, 2020
View reviewed changes
Copy link
Copy Markdown
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
@vincepri vincepri force-pushed the propagate-context-worker branch from 09dd8cc to 0e459aa Compare August 5, 2020 16:30
@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
detiber
detiber reviewed Aug 5, 2020
View reviewed changes
@vincepri vincepri force-pushed the propagate-context-worker branch from 0e459aa to 4b59df8 Compare August 5, 2020 18:00
@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 force-pushed the propagate-context-worker branch from 5ad43b9 to 71825e4 Compare August 6, 2020 14:21
vincepri
vincepri commented Aug 6, 2020
View reviewed changes
@vincepri vincepri force-pushed the propagate-context-worker branch from 71825e4 to 3ba29de Compare August 6, 2020 14:25
@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 4 times, most recently from 42e8e9c to a33241c Compare September 22, 2020 14:59
@vincepri
Copy link
Copy Markdown
Member Author

vincepri commented Sep 22, 2020

/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
Copy Markdown
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.

@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
Copy Markdown
Member Author

vincepri commented Sep 28, 2020

@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
Copy Markdown
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
Copy Markdown
Contributor

ncdc commented Sep 29, 2020

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

@vincepri
Copy link
Copy Markdown
Member Author

vincepri commented Sep 29, 2020 •
edited
Loading

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
Copy Markdown
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>
@vincepri vincepri force-pushed the propagate-context-worker branch from 7131cd8 to 8d45507 Compare October 1, 2020 15:14
@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
Copy Markdown
Contributor

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

@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.

Details

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
Copy Markdown
Member

alvaroaleman commented Oct 1, 2020

/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
@troy0820 troy0820 mentioned this pull request Aug 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanford joelanford joelanford left review comments

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@alexeldeib alexeldeib Awaiting requested review from alexeldeib

@estroz estroz Awaiting requested review from estroz

+2 more reviewers

@DirectXMan12 DirectXMan12 DirectXMan12 left review comments

@detiber detiber detiber left review comments

Reviewers whose approvals may not affect merge requirements

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.

7 participants

@vincepri @k8s-ci-robot @alvaroaleman @ncdc @DirectXMan12 @joelanford @detiber