-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve Context handling in clusterctl #8733
Comments
/triage accepted |
Hello @sbueringer, I would like to work on this but, could you give some more context about this issue, are you talking about everywhere we use |
The scope of the issue is currently intentionally very open. I didn't do further research on which areas of clusterctl we should focus and what the impact on our public interfaces is. But yeah overall we should probably replace all occurences of context.TODO(). I would definitely recommend to open separate PRs to focus each of them on certain code paths or packages though. @ykakarap @fabriziopandini @Jont828 Any opinions from your side? |
Sounds like a good idea. I would recommend starting with some simple wins like evaluating any root level context that are being using and seeing if we can replace them with the right context (context.Background (?) or whatever is reelvant). Refactoring some of the other code to pass through context from top-down might get a little tricky and we should evaluate them on a case by case basis as refactoring some of these functions would mean some of the exported clusterctl interfaces would be impacted. |
Given our current clusterctl compatibility guarantee it is technically okay for us to make the changes to the interface but it would be a nice to have if we can find a way to keep the impact small. |
Since we use a lot of clusterctl functions in Cluster API operator, it would be beneficial for us to support "context" here. I'm going to implement a fix later today to instrument all required functions with "context". |
What would you like to be added (User Story)?
Today we have plenty of places in clusterctl where we use context.TODO(). This issue is about incrementally improving clusterctl to pass through Context top-down.
Related:
Detailed Description
Anything else you would like to add?
No response
Label(s) to be applied
/kind cleanup
/area clusterctl
One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels.
The text was updated successfully, but these errors were encountered: