Use context.Context in API methods

[matejvasek]

Description

Make API functions accept context.Context as the first argument.

Reference

Fixes #1272

[knative-prow-robot]

Welcome @matejvasek! It looks like this is your first PR to knative/client 🎉

[knative-prow-robot]

Hi @matejvasek. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

[matejvasek]

/cc @rhuss @navidshaikh

[matejvasek]

We also need to make changes to the eventing client interface and maybe some others.

[matejvasek]

@rhuss what subcommands of kn typically take more that one second?

[navidshaikh]

/ok-to-test

[matejvasek]

@navidshaikh please another /ok-to-test

[dsimansk]

@navidshaikh prow bot doesn't run the tests on Draft PRs?

[matejvasek]

@dsimansk it did run some integration tests after @navidshaikh set the ok to test label. Latter I committed new stuff, but the integration tests were not run. 🤷‍♂️

[dsimansk]

Well, one ok should be enough, plus there's a label now etc.

/ok-to-test

[matejvasek]

@dsimansk please run test again, I did small fix.
Edit: Nevermind they are running, maybe because I marked PR ready for review.

[matejvasek]

Should I care about that coverage?

[dsimansk]

Should I care about that coverage?

It'd better if there was a test for new signals handling in main.go at least.

I've found only one leftover context.TODO():
https://github.com/knative/client/blob/main/pkg/serving/config_changes.go#L168-L169
used at
https://github.com/knative/client/blob/main/pkg/kn/commands/service/configuration_edit_flags.go#L382

[maximilien]

Truly appreciate the effort. But I’ll also opine how much I hate the pollution that entails, especially for context.TODO(). Oh well, I guess somethings in life you just live with.

Thanks @matejvasek for the hard work on this. Cheers 🍻

[matejvasek]

@maximilien I don't like that pollution too. But if there is no ctx to cancel potentially long running task then it forces users to do stuff like:

func getFoo(ctx context.Context) ([]byte, error) {
	ch := make(chan struct {
		f []byte
		e error
	})

	go func() {
		foo, err := xyz.getFoo()
		ch <- struct {
			f []byte
			e error
		}{f: foo, e: err}
	}()

	select {
	case res := <-ch:
		return res.f, res.e
	case <-ctx.Done():
		return nil, ctx.Err()
	}
}

which is not nice.

In addition many interfaces use ctx as the first arguments, like: docker or k8s APIs.

[rhuss]

I agree that the context passing in the argument list is not nice, but it's golang way of passing context (in contrast to Java where you probably would have used a ThreadLocal for this). And as @matejvasek mentioned, Docker and Kubernetes changed to this style, too, so we are in good company.

[rhuss]

Thanks @matejvasek for the PR, very nice :-) Unfortunately, I don't have spare cycles for a review this week, but that's no loss since @maximilien @navidshaikh @dsimansk are excellent reviewers :)

[matejvasek]

Most of the changes here are mechanical done by IDE/sed. Only functional changes are done to wait/watch logic. Also I added handling for signals that's for CLI, not API part.

[matejvasek]

kubernetes/apimachinery#118

[navidshaikh]

/assign

[navidshaikh]

Thanks!
I've commented inline on a few files, but the following changes need to be done for each client:

1. For each individual clients,

• Remove the context arg for individual Namespace() as it's an accessor (the ns is set via factory while creating the respective client) - for eg: see review for file pkg/messaging/v1beta1/channels_client.go
• Use ctx := context.Background() and pass along the ctx to the chain of calls in client_mock_test.go - for eg: see review for file pkg/dynamic/client_mock_test.go

2. Thanks for taking care of signal handling, can we take it off from this PR and have it in subsequent one ? :-)

3. Please add an entry in changelog https://github.com/knative/client/blob/main/CHANGELOG.adoc

[navidshaikh]

kubernetes/apimachinery # 118

@matejvasek : are you seeing this while updating pkg/wait ? :-)

[matejvasek]

kubernetes/apimachinery # 118

@matejvasek : are you seeing this while updating pkg/wait ? :-)

@navidshaikh yes I do (just sometimes, depending on when cancel happened).
The thing is that resulting error is not wrapped context.Canceled, so errors.Is() won't work on in. I could check for context canceled sub-string in err.Error() but that feels wrong.

[matejvasek]

Should I care about that coverage?

It'd better if there was a test for new signals handling in main.go at least.

I've found only one leftover context.TODO():
https://github.com/knative/client/blob/main/pkg/serving/config_changes.go#L168-L169
used at
https://github.com/knative/client/blob/main/pkg/kn/commands/service/configuration_edit_flags.go#L382

I noticed that, but since it's not in client API I decided to leave it alone.

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/wait/wait_for_ready.go	75.8%	76.3%	0.5

[navidshaikh]

/lgtm

w00t! Thanks a lot @matejvasek for quick turn around on it and adding tests!

Next set of action items would be to update the plugins consuming client APIs, if any.
/cc: @zhanggbj @maximilien @christophd @zhangtbj @dsimansk @rhuss

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh

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:

• OWNERS [navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment