Human-readable revision describe #475

sixolet · 2019-11-04T23:31:26Z

Revision describe based on new format for service describe.

Non-verbose:

$ ./kn revision describe bar-bhndh-34 
Name:         bar-bhndh-34
Namespace:    default
Annotations:  autoscaling.knative.dev/target=-1
Age:          32d
Image:        gcr.io/knative-samples/helloworld-nodejs:latest (pinned to ae8c74)
Env:          ASDF=tame, BAZ=quux
Concurrency:  
  Limit:      1
  Target:     -1
CPU:          400m
Service:      bar

Conditions:  
  OK TYPE                  AGE REASON
  ++ Ready                 32d 
  ++ BuildSucceeded        32d 
  ++ ContainerHealthy      32d 
  ++ ResourcesAvailable    32d 
   I Active                32d NoTraffic

Verbose (note the extra info about service):

$ ./kn revision describe bar-bhndh-34 -v
Name:              bar-bhndh-34
Namespace:         default
Labels:            client.knative.dev/nonce=ygvkcsnvhj
                   serving.knative.dev/configuration=bar
                   serving.knative.dev/configurationGeneration=23
                   serving.knative.dev/service=bar
Annotations:       autoscaling.knative.dev/target=-1
                   client.knative.dev/user-image=gcr.io/knative-samples/helloworld-nodejs:latest
                   serving.knative.dev/lastPinned=1572895120
Age:               32d
Image:             gcr.io/knative-samples/helloworld-nodejs:latest (pinned to ae8c74)
Env:               ASDF=tame
                   BAZ=quux
Concurrency:       
  Limit:           1
  Target:          -1
CPU:               400m
Service:           bar
  Config Gen:      23
  Latest Created:  true
  Latest Ready:    true
  Traffic:         10%
  Tags:            latest

Conditions:  
  OK TYPE                  AGE REASON
  ++ Ready                 32d 
  ++ BuildSucceeded        32d 
  ++ ContainerHealthy      32d 
  ++ ResourcesAvailable    32d 
   I Active                32d NoTraffic (The target is not receiving traffic.)

rhuss

Thanks, looks good to me !

Two points:

We shouldn't print a concurrency limit of 0 imo (it does mean to use the default limit, right ? So can just be left out or replaced by this default limit if we are able to query that)
We should reuse the common code of kn service describe and kn revision describe. But we can leave that for a later PR.

rhuss · 2019-11-05T10:25:56Z

pkg/kn/commands/revision/describe.go

+	if ok {
+		serviceSection := dw.WriteAttribute("Service", serviceName)
+		if printDetails {
+			serviceSection.WriteAttribute("Config Gen", revision.Labels[servingserving.ConfigurationGenerationLabelKey])


As we are in verbose mode anyway, could we expand this to "Configuration Generation" or maybe just "Generation" ? (compromise: "Config Generation")

pkg/kn/commands/describe.go

rhuss · 2019-11-05T10:47:45Z

pkg/kn/commands/revision/describe.go

+}
+
+// Format scale in the format "min ... max" with max = ∞ if not set
+func formatScale(minScale *int, maxScale *int) string {


Should we move that up to a FormatScale, reusable by service describe ?

Oh dang, it looks like I slightly misued git and forgot part of the refactor when I resurrected this PR. Resurrecting that :)

pkg/kn/commands/revision/describe.go

rhuss · 2019-11-05T10:52:59Z

pkg/kn/commands/revision/describe.go

+}
+
+// Write request ... limits or only one of them
+func writeResourcesHelper(dw printers.PrefixWriter, label string, request *resource.Quantity, limit *resource.Quantity) {


This method should be unified with the printing code for service describe.

rhuss · 2019-11-05T10:53:22Z

pkg/kn/commands/revision/describe.go

+	return ret
+}
+
+func stringifyEnv(revision *v1alpha1.Revision) []string {


unify with service describe.

toVersus

A few suggestions, but not necessarily required to take in

toVersus · 2019-11-05T13:49:26Z

pkg/kn/commands/revision/describe.go

+
+	envVars := make([]string, 0, len(container.Env))
+	for _, env := range container.Env {
+		var value string


value := env.Value if env.ValueFrom != nil { value = "[ref]" }

toVersus · 2019-11-05T14:16:34Z

pkg/kn/commands/revision/describe.go

+	var percent int64
+	tags := []string{}
+	for _, target := range service.Status.Traffic {
+		if target.RevisionName == name {


if target.RevisionName != name { continue } if target.Percent != nil { percent += *target.Percent } if target.Tag != "" { tags = append(tags, target.Tag) }

pkg/kn/commands/describe.go

pkg/kn/commands/revision/describe_test.go

maximilien

A few error checks missing in addition to current comments.

Thanks for contribution.

pkg/kn/commands/revision/describe.go

maximilien · 2019-11-05T20:38:43Z

pkg/kn/commands/revision/describe.go

+
+func WriteScale(dw printers.PrefixWriter, revision *v1alpha1.Revision) {
+	// Scale spec if given
+	scale, _ := clientserving.ScalingInfo(&revision.ObjectMeta)


Don't want to nag but ignoring errors here... could easily be handled IMO.

knative-prow-robot · 2019-11-05T20:40:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, 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~~ [maximilien,sixolet]

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

maximilien · 2019-11-05T20:41:33Z

/retest

knative-metrics-robot · 2019-11-05T23:26:08Z

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/kn/commands/describe.go	82.7%	73.1%	-9.6
pkg/kn/commands/revision/describe.go	78.3%	57.5%	-20.7
pkg/kn/commands/service/describe.go	83.7%	79.9%	-3.8
pkg/serving/revision_template.go	87.5%	64.7%	-22.8

maximilien · 2019-11-06T01:42:30Z

/lgtm

sixolet added 2 commits November 4, 2019 14:59

Revision describe rebaseable

2208daa

Fix up for rebase

fcf3b6a

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

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 4, 2019

knative-prow-robot requested review from cppforlife and navidshaikh November 4, 2019 23:31

sixolet added 2 commits November 4, 2019 18:13

Some tests

380aed7

moretests

c00cef9

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 5, 2019

rhuss requested changes Nov 5, 2019

View reviewed changes

toVersus reviewed Nov 5, 2019

View reviewed changes

Forgot part of the refactor!

707db15

maximilien approved these changes Nov 5, 2019

View reviewed changes

sixolet added 2 commits November 5, 2019 14:16

Fix unit tests

8edd5d3

Change e2e tests and respond to feedback

545ab88

knative-prow-robot assigned maximilien Nov 6, 2019

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

knative-prow-robot merged commit 4874b9a into knative:master Nov 6, 2019

navidshaikh mentioned this pull request Nov 6, 2019

Implement kn revision describe command #206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Human-readable revision describe #475

Human-readable revision describe #475

sixolet commented Nov 4, 2019

rhuss left a comment

rhuss Nov 5, 2019

sixolet Nov 5, 2019

rhuss Nov 5, 2019

sixolet Nov 5, 2019

rhuss Nov 5, 2019

rhuss Nov 5, 2019

toVersus left a comment

toVersus Nov 5, 2019

toVersus Nov 5, 2019

maximilien left a comment •

edited

Loading

maximilien Nov 5, 2019

knative-prow-robot commented Nov 5, 2019

maximilien commented Nov 5, 2019

knative-metrics-robot commented Nov 5, 2019

maximilien commented Nov 6, 2019

Human-readable revision describe #475

Human-readable revision describe #475

Conversation

sixolet commented Nov 4, 2019

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toVersus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 5, 2019

maximilien commented Nov 5, 2019

knative-metrics-robot commented Nov 5, 2019

maximilien commented Nov 6, 2019

maximilien left a comment •

edited

Loading