Adds service get command #90

navidshaikh · 2019-05-03T10:52:52Z

Fixes #40

./kn service get --all-namespaces
NAME   DOMAIN                   GENERATION   AGE     CONDITIONS   READY       REASON
s1     s1.default.example.com   4            2d19h   3 OK / 3     True        
s2     s2.default.example.com   1            17h     0 OK / 3     False       RevisionMissing : Configuration "s2" does not have any ready Revision.

knative-prow-robot · 2019-05-03T10:53:07Z

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

pkg/printers/printers.go

pkg/kn/commands/service_get_flags.go

navidshaikh · 2019-05-08T19:03:13Z

@cppforlife : Addressed your review comments PTAL, if the structure looks good, I will add some unit tests.

pkg/kn/commands/humanreadable_flags.go

pkg/kn/commands/service_get_flags.go

sixolet · 2019-05-09T22:36:32Z

/ok-to-test

rhuss · 2019-05-10T13:25:59Z

After analysing the test errors, I think we have a more serious issue here when we want to use the pkg/printers from cli-runtime:

pkg/printers were only introduced in cli-runtime since kubernetes-1.14.0
knative/serving depends on kubernetes-1.12.6
client-go (used by both, serving and cli-runtime) is incompatible between Kubernetes 1.12 and 1.14 (that's indicated in the CI test error, where client-go latest is used).

The two options, to try cli-runtime with client-go 1.12 or to try knative-serving with client-go 1.14 both failed.

Afais, we have two options if we want to reuse the functionality of the printers package in cli-runtime:

Convince knative-serving to upgrade to Kubernetes 1.14. This is probably not something which can be done quickly.
Copy over printers package from cli-runtime here temporarily, and switch over to cli-runtim when knative-serving has upgraded.

But maybe there are other solutions ?

Maybe we should also reconsider the re-usage of cli-runtime as this shared dependency (both cli-runtime and knative are depending on client-go) will likely cause issues in the future again, considering the different lifecycles between knative and kubernetes (and hence cli-runtime).

@cppforlife @sixolet wdyt ?

replaces service list command

navidshaikh · 2019-05-13T10:48:12Z

@rhuss : I think there was a mess up for vendors files due to multiple rebases (this PR was rebased onto another one, which is now merged). I reset hard to point to master and added / updated the files in this PR to create fresh commits, the failure is gone!

knative-metrics-robot · 2019-05-13T12:34:07Z

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
pkg/kn/commands/human_readable_flags.go	Do not exist	80.0%
pkg/kn/commands/service_get.go	Do not exist	80.0%
pkg/kn/commands/service_get_flags.go	Do not exist	63.6%

rhuss · 2019-05-13T13:14:12Z

@navidshaikh I think so, too. I was a bit confused about the reference to cli-runtime/printers as I thought this was a hard dependency for the knative client code itself. However, this doesn't seems to be the case. Also it seems that the cli-runtime/genericclioptions/printers in 1.12 has been moved to cli-runtime/printers in 1.14 (I overlooked that).

So, if there is no direct usage of 1.14 printers, then there is also no two client-go vesions required.

Looks all good to me now, and sorry for the noise ...

navidshaikh · 2019-05-13T14:23:26Z

pkg/kn/commands/service_get_flags.go

+
+// ToPrinter attempts to find a composed set of ServiceGetFlags suitable for
+// returning a printer based on current flag values.
+func (f *ServiceGetFlags) ToPrinter() (hprinters.ResourcePrinter, error) {


@rhuss : There is one thing that I have updated, changed the reference for ResourcePrinter from printers.ResourcePrinter defined here to hprinters.ResourcePrinter defined in client here.

Yes, that was it. As long as you don't have a reference into 'cli-runtime/pkg/printers' anymore, there is no need for 'go modules' to add a newer version of cli-runtime (i.e. 1.14) which conflicts with serving 0.5.2 (and k8s 1.12).

So this is actually the important change and of course a better fix then copying over stuff.

rhuss

lgtm

knative-prow-robot · 2019-05-13T15:30:16Z

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

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

sixolet · 2019-05-13T17:56:27Z

/lgtm

* Adds service get command replaces service list command * Updates go.mod and vendors/* * Adds message if no services found in requested namespace * Adds tests for service get

knative-prow-robot requested review from brendandburns, pmorie, smarterclayton and thockin May 3, 2019 10:53

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 3, 2019

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 3, 2019

This was referenced May 3, 2019

Adds more columns in revision get command output #70

Closed

Adds human-friendly table printing for resource listing #85

Merged

navidshaikh force-pushed the new-service-get branch from d801da9 to 1f09723 Compare May 7, 2019 10:45

cppforlife reviewed May 7, 2019

View reviewed changes

pkg/printers/printers.go Outdated Show resolved Hide resolved

navidshaikh force-pushed the new-service-get branch from 1f09723 to 67b3c2a Compare May 8, 2019 11:33

navidshaikh commented May 8, 2019

View reviewed changes

pkg/kn/commands/service_get_flags.go Show resolved Hide resolved

cppforlife reviewed May 8, 2019

View reviewed changes

pkg/kn/commands/humanreadable_flags.go Outdated Show resolved Hide resolved

navidshaikh mentioned this pull request May 9, 2019

Adds more columns in service get command output #56

Closed

navidshaikh force-pushed the new-service-get branch from 67b3c2a to 32b0864 Compare May 9, 2019 11:22

sixolet approved these changes May 9, 2019

View reviewed changes

pkg/kn/commands/humanreadable_flags.go Outdated Show resolved Hide resolved

pkg/kn/commands/service_get_flags.go Show resolved Hide resolved

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2019

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 9, 2019

maximilien mentioned this pull request May 10, 2019

Adds basic workflow #99

Merged

navidshaikh added 2 commits May 13, 2019 16:05

Adds service get command

0ab7020

replaces service list command

Updates go.mod and vendors/*

662a562

navidshaikh force-pushed the new-service-get branch from 32b0864 to 662a562 Compare May 13, 2019 10:37

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 13, 2019

navidshaikh added 2 commits May 13, 2019 16:36

Adds message if no services found in requested namespace

0ba3a1e

Adds tests for service get

115229a

navidshaikh commented May 13, 2019

View reviewed changes

rhuss approved these changes May 13, 2019

View reviewed changes

knative-prow-robot assigned sixolet May 13, 2019

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

knative-prow-robot merged commit bd7d57f into knative:master May 13, 2019

sixolet mentioned this pull request May 14, 2019

service get #48

Closed

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

Fix link (knative#90)

95bd6f4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds service get command #90

Adds service get command #90

navidshaikh commented May 3, 2019

knative-prow-robot commented May 3, 2019

navidshaikh commented May 8, 2019

sixolet commented May 9, 2019

rhuss commented May 10, 2019

navidshaikh commented May 13, 2019 •

edited

Loading

knative-metrics-robot commented May 13, 2019

rhuss commented May 13, 2019

navidshaikh May 13, 2019

rhuss May 13, 2019

rhuss left a comment

knative-prow-robot commented May 13, 2019

sixolet commented May 13, 2019

Adds service get command #90

Adds service get command #90

Conversation

navidshaikh commented May 3, 2019

knative-prow-robot commented May 3, 2019

navidshaikh commented May 8, 2019

sixolet commented May 9, 2019

rhuss commented May 10, 2019

navidshaikh commented May 13, 2019 • edited Loading

knative-metrics-robot commented May 13, 2019

rhuss commented May 13, 2019

navidshaikh May 13, 2019

Choose a reason for hiding this comment

rhuss May 13, 2019

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented May 13, 2019

sixolet commented May 13, 2019

navidshaikh commented May 13, 2019 •

edited

Loading