refactor(serving): KnClient interface for single point of cluster access #134

rhuss · 2019-05-20T19:12:30Z

Fixes #133.

Introducing a KnClient interface through which all access to the k8s cluster should happen. This facade takes care that GVK are properly set, simplifies the usage and provide domain-specific methods (like GetRevisionsForService(string).

For testing, it uses https://godoc.org/gotest.tools but otherwise plain testing. gotest.tools is reasonably lightweight and is already used in other knative projects (like pkg iirc).

Having this kind of clear separation and single-point of cluster contact also will make the transition easier when switching to newer API versions.

You find the test code in https://github.com/rhuss/client/blob/pr/add-schema-handling/pkg/serving/v1alpha1/client_test.go

Follow the discussion overthere whether to pick up this PR or #214

knative-prow-robot · 2019-05-20T19:12:45Z

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

matzew · 2019-05-20T20:06:24Z

/ok-to-test

rhuss · 2019-05-20T20:45:46Z

/test all

rhuss · 2019-05-20T20:45:59Z

/joke

knative-prow-robot · 2019-05-20T20:46:00Z

@rhuss: What do you call an Argentinian with a rubber toe? Roberto

In response to this:

/joke

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.

pkg/serving/schema_handling_test.go

pkg/kn/commands/revision_describe.go

pkg/serving/schema_handling.go

pkg/serving/schema_handling_test.go

cppforlife · 2019-05-21T16:50:39Z

@rhuss i forget why do we even set this on retrieved objects, dont they already have this on them?

rhuss · 2019-05-21T17:22:19Z

@rhuss i forget why do we even set this on retrieved objects, dont they already have this on them?

No, the recieved objects from the RestClient don't have the GVK set by intention so that the field is empty.

In memory, these fields are empty and will be filled only during marshalling to JSON/protobuf (using the same mechanism as here). That is automatically done by client-go when reaching out to the server.

That seems to be a global convention throughout the go-client.

(BTW, I have this info from first-hand experience where I struggled quite a bit with this, but it is also described in the forthcoming "Programming Kubernetes" book by @stts . Excellent book, I learned a lot already from the early access version.)

rhuss · 2019-05-24T14:57:05Z

@cppforlife @sixolet I addressed all review comments. Can we please get this merged asap, as this affects all other API calls (like the one I'm just working on for kn service describe) ?

cppforlife · 2019-05-30T18:35:33Z

@rhuss hmm. i was looking around kubernetes issues/source and found a few issues related to these fields being cleared out just to avoid duplicating data from the struct type itself. kind of ugly...

it seems that we need this info there just for printing. i think we should create a new printer (implements ResourcePrinter interface) that would backfill this info before passing on obj to underlying printer. im not a big fan of having public UpdateGroupVersionKind method added all over the place.

rhuss · 2019-05-30T19:06:13Z

it seems that we need this info there just for printing. i think we should create a new printer (implements ResourcePrinter interface) that would backfill this info before passing on obj to underlying printer. im not a big fan of having public UpdateGroupVersionKind method added all over the place.

I see your point, but I'm afraid that we have to do this all the time when we read resources to present it to the user in any way (so everything except for create/update/delete). I wouldn't put this into the printer directly, as there might be occasions where we show resource without printers. But yes, we could go this way (as I think its really only needed for machine-readable output, but this is delegated to cli-runtime).

An alternative approach is to collect all server-side access into a thin layer, which is suited for the client (and which could be also used as a kn-client library in other client, too).

An example for such layer is part of #75 , in https://github.com/knative/client/blob/a44413cab56cc4fc568e5c02c09f6f2aa1ebc958/pkg/serving/client.go

It's kind of a DAO layer (you see where I'm coming from :), which encapsulates nicely client specific context information and provides specialized method which suit the client's use case, like

client/pkg/serving/client.go

Line 56 in a44413c

    
           func (cl *NamespacedClient) RevisionsForService(service *v1alpha1.Service) ([]*v1alpha1.Revision, error) {

returns all revisions for a specific service (you can't do this directly with standard serving API).

If all server access would go through this specialized client, then this is the single place to update the GVK.

rhuss · 2019-05-30T19:08:26Z

Said that, let me try to put this into the printer layer and looks how it feels.

This is an alternate solution to knative#134 for fixing knative#133. In this commit the update of the GVK coordinates are moved to the resource printer.

rhuss · 2019-05-30T20:00:31Z

/assign cppforlife

sixolet

I've now had a chance to look at #153 and this PR in detail, and come to the conclusion that the GVK should come from ServingFactory, along with the client object. That'll preserve the flexibility we need to add a --v1beta1 flag later. Can we do that?

sixolet · 2019-05-31T19:02:06Z

Other than that, I like this approach more than #153 ; it doesn't seem like a ResourcePrinter's concern to mutate the resources in question.

rhuss · 2019-07-02T18:22:47Z

I think we are good now, now that we have decided on the testing framework. thanks to everyone ;-)

'would be happy when this old-timer (on which implementation we agreed at some point in time) could be merged asap (rebasing gets harder and harder). Happy to help to adapt outstanding PRs for using the client layer or fix any issue and refinements afterwards. // @sixolet @cppforlife @maximilien

knative-metrics-robot · 2019-07-02T18:29:31Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/namespaced.go	87.5%	72.7%	-14.8
pkg/kn/commands/revision/revision_describe.go	79.2%	78.3%	-0.9
pkg/kn/commands/revision/revision_list.go	81.2%	80.6%	-0.7
pkg/kn/commands/service/service_create.go	69.0%	68.6%	-0.4
pkg/kn/commands/service/service_describe.go	79.2%	78.3%	-0.9
pkg/kn/commands/service/service_list.go	80.0%	79.2%	-0.8
pkg/serving/schema_handling.go	Do not exist	91.7%
pkg/serving/v1alpha1/client.go	Do not exist	84.3%

rhuss · 2019-07-02T18:33:43Z

hmm, next conflict to resolve (not the modules one, but I have to refactor routes/list.go. I make three crosses when this PR is merged ;-)

knative-metrics-robot · 2019-07-02T19:47:28Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/namespaced.go	87.5%	72.7%	-14.8
pkg/kn/commands/revision/revision_describe.go	79.2%	78.3%	-0.9
pkg/kn/commands/revision/revision_list.go	81.2%	80.6%	-0.7
pkg/kn/commands/route/list.go	76.7%	75.0%	-1.7
pkg/kn/commands/service/service_create.go	80.8%	80.6%	-0.3
pkg/kn/commands/service/service_describe.go	79.2%	78.3%	-0.9
pkg/kn/commands/service/service_list.go	80.0%	79.2%	-0.8
pkg/serving/schema_handling.go	Do not exist	91.7%
pkg/serving/v1alpha1/client.go	Do not exist	86.0%

knative-metrics-robot · 2019-07-02T19:48:40Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/namespaced.go	87.5%	72.7%	-14.8
pkg/kn/commands/revision/revision_describe.go	79.2%	78.3%	-0.9
pkg/kn/commands/revision/revision_list.go	81.2%	80.6%	-0.7
pkg/kn/commands/route/list.go	76.7%	75.0%	-1.7
pkg/kn/commands/service/service_create.go	80.8%	80.6%	-0.3
pkg/kn/commands/service/service_describe.go	79.2%	78.3%	-0.9
pkg/kn/commands/service/service_list.go	80.0%	79.2%	-0.8
pkg/serving/schema_handling.go	Do not exist	91.7%
pkg/serving/v1alpha1/client.go	Do not exist	86.0%

rhuss · 2019-07-02T21:07:14Z

ok, here we go. // @sixolet @cppforlife anything still missing ?

To recapitulate, the core part is the refactoring leading to the following KnClient interface:

type KnClient interface {


	// Get a service by its unique name
	GetService(name string) (*v1alpha1.Service, error)


	// List services
	ListServices(opts ...ListConfig) (*v1alpha1.ServiceList, error)


	// Create a new service
	CreateService(service *v1alpha1.Service) error


	// Update the given service
	UpdateService(service *v1alpha1.Service) error


	// Delete a service by name
	DeleteService(name string) error


	// Wait for a service to become ready, but not longer than provided timeout
	WaitForService(name string, timeout time.Duration, out io.Writer) error


	// Get a revision by name
	GetRevision(name string) (*v1alpha1.Revision, error)


	// List revisions
	ListRevisions(opts ...ListConfig) (*v1alpha1.RevisionList, error)


	// Delete a revision
	DeleteRevision(name string) error


	// List routes
	ListRoutes(opts ...ListConfig) (*v1alpha1.RouteList, error)
}

in https://github.com/rhuss/client/blob/f264ae6f1ff6b6b816d30274f7cab096dd29066e/pkg/serving/v1alpha1/client.go#L38-L69

This is specific to v1alhpa1 but we could easily provide a similar interface for v1beta1. Then the callers can dispatch on the current cluster version to decide which version to use. The WaitForService could be a bit nicer without an io.Writer argument (i.e. with just a ProgressHandler callback function which in turn can print out th e progress). I would address this in a subsequent PR.

The interface is deliberately kept easy (e.g without namespace params as this is the context stored during creation of the implementation, and also no return of the created service object) so that it could easily serve as an API for other users (like e.g. plugins which want to combine knative functionality with something else).

For list methods a fluid config model is implemented to easily define a filter (e.g. ListRevisions(WithService("myservice")))

Really I would be very happy if we get that merged soon and I'm totally comitted to make it better and improvements after this. But now, for every PR coming in, I have again to refactor during a rebase to use this model (e.g. like I just did for ListRoutes()). It would be much better if people already could jump on this new API right now.

Ah yes, testing will becomes much easier, too soon (as only this client needs to deal with a fake service) whereas the command test can jump on a test implementation of this simple interface.

Please.

knative-metrics-robot · 2019-07-03T07:02:08Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/namespaced.go	87.5%	72.7%	-14.8
pkg/kn/commands/revision/revision_describe.go	79.2%	78.3%	-0.9
pkg/kn/commands/revision/revision_list.go	81.2%	80.6%	-0.7
pkg/kn/commands/route/list.go	76.7%	75.0%	-1.7
pkg/kn/commands/service/service_create.go	80.8%	80.6%	-0.3
pkg/kn/commands/service/service_describe.go	79.2%	78.3%	-0.9
pkg/kn/commands/service/service_list.go	80.0%	79.2%	-0.8
pkg/serving/schema_handling.go	Do not exist	91.7%
pkg/serving/v1alpha1/client.go	Do not exist	86.0%

This remove the io.Writer from the client interface and replaces it with ProgressHandler interface. A SimpleProgressHandler instance can be used to print out the wait progress on a writer. This commit relies on that being PR knative#134 merged first.

Fixes knative#133.

…options

knative-metrics-robot · 2019-07-08T06:57:56Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/namespaced.go	87.5%	72.7%	-14.8
pkg/kn/commands/revision/revision_describe.go	79.2%	78.3%	-0.9
pkg/kn/commands/revision/revision_list.go	81.2%	80.6%	-0.7
pkg/kn/commands/route/list.go	76.7%	75.0%	-1.7
pkg/kn/commands/service/service_create.go	80.8%	80.6%	-0.3
pkg/kn/commands/service/service_describe.go	79.2%	78.3%	-0.9
pkg/kn/commands/service/service_list.go	80.0%	79.2%	-0.8
pkg/serving/schema_handling.go	Do not exist	91.7%
pkg/serving/v1alpha1/client.go	Do not exist	86.0%

sixolet · 2019-07-08T16:40:12Z

/approve
/lgtm

knative-prow-robot · 2019-07-08T16:40:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh, rhuss, sixolet

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~~ [rhuss,sixolet]

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

* fix: Check if release branch exists * Reflect review suggestion --------- Co-authored-by: David Simansky <[email protected]>

knative-prow-robot requested review from evankanderson and mattmoor May 20, 2019 19:12

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 20, 2019

rhuss force-pushed the pr/add-schema-handling branch from 0cbcdd9 to ba6f7d5 Compare May 20, 2019 19:25

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 20, 2019

rhuss force-pushed the pr/add-schema-handling branch from ba6f7d5 to 7ace8a4 Compare May 20, 2019 20:08

navidshaikh reviewed May 21, 2019

View reviewed changes

navidshaikh approved these changes May 22, 2019

View reviewed changes

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 24, 2019

rhuss mentioned this pull request May 30, 2019

get a particular service and revision based on name parameter #150

Merged

rhuss mentioned this pull request May 30, 2019

fix(serving): Update GVK on objects within a ResourcePrinter. #153

Closed

knative-prow-robot assigned cppforlife May 30, 2019

sixolet suggested changes May 31, 2019

View reviewed changes

rhuss force-pushed the pr/add-schema-handling branch from 6df495e to ebae301 Compare May 31, 2019 20:46

rhuss force-pushed the pr/add-schema-handling branch from 22db311 to 0db1b15 Compare July 2, 2019 18:17

rhuss removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2019

rhuss force-pushed the pr/add-schema-handling branch 2 times, most recently from cf01215 to f264ae6 Compare July 2, 2019 19:46

rhuss changed the title ~~refactor(serving): KnClient interface for single point of cluster access [gotest.tools version]~~ refactor(serving): KnClient interface for single point of cluster access Jul 2, 2019

maximilien mentioned this pull request Jul 3, 2019

Support multi config files #222

Merged

rhuss force-pushed the pr/add-schema-handling branch from f264ae6 to f7e7a79 Compare July 3, 2019 07:00

rhuss mentioned this pull request Jul 5, 2019

Fix issue #205 to add route describe feature and test #228

Closed

navidshaikh approved these changes Jul 5, 2019

View reviewed changes

rhuss mentioned this pull request Jul 5, 2019

refactor(service): Add a ProgressHandler for the Wait interface #234

Closed

rhuss added 4 commits July 8, 2019 08:50

fix(serving): Remove hardcoded GVK and look it up from schema

81e6b76

Fixes knative#133.

chore(serving): Add test for "WaitForService()"

1c04eed

refactor(service): Add listRoutes() to client + generic way for list …

4a80ff4

…options

chore(serving): Fixing rebase conflicts

0fb9770

rhuss force-pushed the pr/add-schema-handling branch from f7e7a79 to 0fb9770 Compare July 8, 2019 06:56

rhuss added this to the v0.2.0 milestone Jul 8, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2019

knative-prow-robot merged commit 98184ea into knative:master Jul 8, 2019

dsimansk added a commit to dsimansk/client that referenced this pull request Oct 2, 2023

[release-v1.10] fix: Check if release branch exists (knative#134)

8f29236

* fix: Check if release branch exists * Reflect review suggestion --------- Co-authored-by: David Simansky <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(serving): KnClient interface for single point of cluster access #134

refactor(serving): KnClient interface for single point of cluster access #134

rhuss commented May 20, 2019 •

edited

Loading

knative-prow-robot commented May 20, 2019

matzew commented May 20, 2019

rhuss commented May 20, 2019

rhuss commented May 20, 2019

knative-prow-robot commented May 20, 2019

cppforlife commented May 21, 2019

rhuss commented May 21, 2019

rhuss commented May 24, 2019

cppforlife commented May 30, 2019

rhuss commented May 30, 2019 •

edited

Loading

rhuss commented May 30, 2019

rhuss commented May 30, 2019

sixolet left a comment

sixolet commented May 31, 2019

rhuss commented Jul 2, 2019

knative-metrics-robot commented Jul 2, 2019

rhuss commented Jul 2, 2019

knative-metrics-robot commented Jul 2, 2019

knative-metrics-robot commented Jul 2, 2019

rhuss commented Jul 2, 2019 •

edited

Loading

knative-metrics-robot commented Jul 3, 2019

knative-metrics-robot commented Jul 8, 2019

sixolet commented Jul 8, 2019

knative-prow-robot commented Jul 8, 2019

refactor(serving): KnClient interface for single point of cluster access #134

refactor(serving): KnClient interface for single point of cluster access #134

Conversation

rhuss commented May 20, 2019 • edited Loading

knative-prow-robot commented May 20, 2019

matzew commented May 20, 2019

rhuss commented May 20, 2019

rhuss commented May 20, 2019

knative-prow-robot commented May 20, 2019

cppforlife commented May 21, 2019

rhuss commented May 21, 2019

rhuss commented May 24, 2019

cppforlife commented May 30, 2019

rhuss commented May 30, 2019 • edited Loading

rhuss commented May 30, 2019

rhuss commented May 30, 2019

sixolet left a comment

Choose a reason for hiding this comment

sixolet commented May 31, 2019

rhuss commented Jul 2, 2019

knative-metrics-robot commented Jul 2, 2019

rhuss commented Jul 2, 2019

knative-metrics-robot commented Jul 2, 2019

knative-metrics-robot commented Jul 2, 2019

rhuss commented Jul 2, 2019 • edited Loading

knative-metrics-robot commented Jul 3, 2019

knative-metrics-robot commented Jul 8, 2019

sixolet commented Jul 8, 2019

knative-prow-robot commented Jul 8, 2019

rhuss commented May 20, 2019 •

edited

Loading

rhuss commented May 30, 2019 •

edited

Loading

rhuss commented Jul 2, 2019 •

edited

Loading