Skip to content

misc(summaryInformer): make ListWatcher context-aware#668

Open
aruiz14 wants to merge 2 commits into
rancher:mainfrom
aruiz14:list-watch-context-aware
Open

misc(summaryInformer): make ListWatcher context-aware#668
aruiz14 wants to merge 2 commits into
rancher:mainfrom
aruiz14:list-watch-context-aware

Conversation

@aruiz14

@aruiz14 aruiz14 commented May 8, 2026

Copy link
Copy Markdown
Contributor

ListFunc and WatchFunc are marked as deprecated (context.TODO() was already a hint), in favor of context-aware equivalents.

@aruiz14 aruiz14 requested a review from a team as a code owner May 8, 2026 15:29

@brandond brandond left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wangler in general makes excessive use of context.TODO(); when context was introduced to the various client-go calls we just punted instead of making the hard change to plumb context through as we should have.

@brandond brandond left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While you're here, why not do the other ListWatch a couple lines above in NewFilteredSummaryInformerWithOptions?

@tomleb

tomleb commented May 8, 2026

Copy link
Copy Markdown
Contributor

Wangler in general makes excessive use of context.TODO(); when context was introduced to the various client-go calls we just punted instead of making the hard change to plumb context through as we should have.

@brandond Not 100% sure, but I think the reason for this is because wrangler was semver (now semver-ish) so using new context-aware methods / fields would break old codebases if you were using pinning to pin k8s to older version.

We've talked a few times now about making wrangler making wrangler basically just follow k8s in terms of Go and k8s libraries.. I still haven't "done" that yet, not sure if it requires a RFD of some kind.. But I think we should just do it because we keep coming back to the same annoying and confusing rules of "well, does this break an older version of rancher / k3s if we're pinning old version.

@brandond

brandond commented May 8, 2026

Copy link
Copy Markdown
Member

Yeah, I guess my point is we (Darren) should have made a breaking change to wrangler when upstream client-go made their breaking change. Instead we had Wrangler just inject TODO everywhere and now it's years later and there is MORE downstream code to fix, not less.

I'm just whinging.

Comment thread pkg/summary/informer/informer.go
@aruiz14 aruiz14 requested a review from brandond May 11, 2026 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants