fix: pass context throughout applier #656
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @sdowell. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
sdowell
force-pushed
the
pass-context
branch
from
39fbd3e to
07190ad
Compare
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -49,7 +51,7 @@ func (i *DeleteOrUpdateInvTask) Start(taskContext *taskrunner.TaskContext) { | |||
| go func() { | |||
| var err error | |||
| if i.Destroy && i.destroySuccessful(taskContext) { | |||
| err = i.deleteInventory() | |||
| err = i.deleteInventory(taskContext.Context()) | |||
There was a problem hiding this comment.
hmm. should we make deleteInventory take a taskContext to be consistent with updateInventory?
There was a problem hiding this comment.
This was the only one I saw, but please double check that all the other interfaces have consistent use of either the TaskContext or the Context.
There was a problem hiding this comment.
updateInventory actually uses other fields from the taskContext, which is why it needs it. deleteInventory just needs the context.Context. It's such a tiny function it doesn't seem like it matters to me.
There was a problem hiding this comment.
You know I love consistency, but I agree it's a small enough that it's not a blocker.
There were various spots in the applier that were not propagating the context and instead just using an empty context. This change fixes all such locations so that the parent context is always honored.
sdowell
force-pushed
the
pass-context
branch
from
07190ad to
35d2761
Compare
|
/retest |
| @@ -40,12 +41,12 @@ type Storage interface { | |||
| GetObject() (*unstructured.Unstructured, error) | |||
| // Apply applies the inventory object. This utility function is used | |||
| // in InventoryClient.Merge and merges the metadata, spec and status. | |||
| Apply(dynamic.Interface, meta.RESTMapper, StatusPolicy) error | |||
| Apply(context.Context, dynamic.Interface, meta.RESTMapper, StatusPolicy) error | |||
There was a problem hiding this comment.
This will require updating the RG inventory client in order to pull this change into Config Sync (or kpt). So we won't be able to pull this in until we either merge my third_party fork or your inventory client rewrite.
Are we ready to do that now? Or should we line up those PRs to be a little closer to landing?
If waiting is the better option, you could just pull this file's changes out to a different PR and merge the rest of this one.
There was a problem hiding this comment.
I'm not planning to upgrade cli-utils in Config Sync until the next change goes in (#653), which completely changes these interfaces.
My intent here was to fix the context issue as an incremental change before the complete interface rewrite. Alternatively we could fix the context issue as part of the interface rewrite or as a subsequent change. I'm fine either way.
If we submit this change and then for some reason need to immediately bump the cli-utils version, we could always update the kpt client implementation to pass a context through.
There was a problem hiding this comment.
Alright.
I'm gonna extract my third_party fork to its own PR anyway. So that will unblock using these as soon as monday.
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: karlkfi, sdowell 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 |
k8s-ci-robot
added
the
approved
There were various spots in the applier that were not propagating the context and instead just using an empty context. This change fixes all such locations so that the parent context is always honored.