feature(#580): adds traffic and tags information to revision list #581

maximilien · 2019-12-20T02:16:52Z

Fixes #580

Proposed Changes

adds traffic information of revision as an annotation in revision list
adds tags information of revision as an annotation in revision list
show traffic and tags when listing revisions

With this change, invocation of kn revision list will look like:

➜  client git:(issue580) ✗ ./kn revision list
Using kn config file: /Users/maximilien/.kn/config.yaml
NAME                  SERVICE       GENERATION   AGE   CONDITIONS   READY   TRAFFIC   TAGS    REASON
twitter-fn-ycybl-1    twitter-fn    26           28d   3 OK / 4     True    100%
summary-fn-hwkll-31   summary-fn    20           23d   3 OK / 4     True    50%       async
summary-fn-qwfdb-1    summary-fn    19           23d   3 OK / 4     True    50%       sync
watson-fn-wxpyf-15    watson-fn     15           28d   3 OK / 4     True    100%
hello-world-tndhb-2   hello-world   2            28d   3 OK / 4     True    100%

➜  client git:(issue580) ✗ ./kn revision list -s summary-fn
Using kn config file: /Users/maximilien/.kn/config.yaml
NAME                  SERVICE      GENERATION   AGE   CONDITIONS   READY   TRAFFIC   TAGS    REASON
summary-fn-hwkll-31   summary-fn   20           23d   3 OK / 4     True    50%       async
summary-fn-qwfdb-1    summary-fn   19           23d   3 OK / 4     True    50%       sync

maximilien · 2019-12-20T02:17:05Z

/ok-to-test

daisy-ycguo · 2019-12-20T08:34:06Z

/test pull-knative-client-integration-tests

rhuss

some minor change requests, othewise looks good. Thank you !

pkg/kn/commands/revision/list.go

rhuss · 2019-12-20T11:40:59Z

pkg/kn/commands/revision/list.go

+
+				traffic, tags := trafficForRevision(revision.Name, service)
+				revision.Annotations[RevisionTrafficAnnotation] = fmt.Sprintf("%d%%", traffic)
+				revision.Annotations[RevisionTagsAnnotation] = strings.Join(tags, ",")


Can we avoid modifying the revision here by e.g. using a map keyed by revision name ? Imagine if someone later would introduce such annotations for real (so that you would override it here). Also 'misusing' an object by writing to it in a pure read-only use case is not architectural sound.

Thought of that, however, the revision is not modified permanently. Only temporarily to hold values that frankly belongs to it. These modified revision objects are in memory and are never seen by 'anyone' else other than the user that invoked the command.

Can we avoid modifying the revision here by

+1

We can use trafficForRevision in printRevision to fetch the traffic and tags.

Read my comment @navidshaikh it does not modify the revision except in memory which makes the rest of the code super clean

Sure! I think this information doesn't belong to Revision and we shouldn't piggyback this data onto Revision.

Sure! I think this information doesn't belong to Revision and we shouldn't piggyback this data onto Revision.

It does at least when using the revision list. See why I opened this issue in the first place. It comes from the need to see this information and not having to have to show the details of the service and parsing the out to find the data. This is my feedback from real usage.

(Imagine someone would print out the full object somewhere in between, like in a kn revision list --verbose or something. You will see artifical annotations here).

kn revision list --verbose call does not exist. If we decide to do it we can easily filter out the annotations or better flag them as such. Which I'd be happy to also do here.

I disagree since annotations are meant to do things like that. However, I am happy to hear suggestions on how else to do it?

That's where I disagree, i.e. that annotations are used for volatile information which never gets persisted ;-). But maybe we could at least agree that listing revisions is a read-only operation and could be implemented with immutable objects that don't allow mutating operations? Why should we break that without a need ?

kn revision list --verbose call does not exist. If we decide to do it we can easily filter out the annotations or better flag them as such. Which I'd be happy to also do here.

That's not really the point, whether this feature exists or not as its just an illustrative example. When you say one can easily add a filter, that is a bad smell, too, to use another hack to fix the previous hack.

Not sure whether I could convince you that keeping certain architectural constraints even when they are not enforced today is a good thing. So let's merge them at let me make an alternative suggestion in the next PR.

Not sure whether I could convince you that keeping certain architectural constraints even when they are not enforced today is a good thing.

That's not the part that I disagree with.

That's where I disagree, i.e. that annotations are used for volatile information which never gets persisted ;-).

Yup, that's the conundrum. It's the use of annotations. For me, I feel that this is a good use since the attributes I am adding traffic and tags are not really "features" of the Service nor the Revision but of both. There are synthesized from and belong to the association between Service and Revision, in my view.

So let's merge them at let me make an alternative suggestion in the next PR.

OK cool. Look forward to it. Hopefully it's not a significant change since that was where I was headed until this approach. But maybe I am missing some easy trick... Happy to learn.

OK cool. Look forward to it. Hopefully it's not a significant change since that was where I was headed until this approach. But maybe I am missing some easy trick... Happy to learn.

I have to apologize a bit :) Ideally I would use classical "view object" used for printing. But the printer framework we are using insist on runtime.Object. To implement those is even more hackish than adding to the annotations (I already was there before I returned back to your approach, which definitely is the better in this particular case). So I suggest indeed to stick to this technique for tunneling purposes (if we would have the printing of a list in our hands, its trivial to change with a revisionInfo object containing the revision + tags,traffic from the service).

However, my point is illustrated quite nicely what happens at the moment, when you are doing a kn revision list -o yaml. This includes, in YAML, the temporary annotations. This is incorrect as these annotations are not part of the revision. We can fix that, but in this initial PR it was overlooked. Something similar can happen in the future, too. Hopefully not, but anybody touching this code needs to be aware of this particular usage of volatile information attached to an otherwise persisted object.

So, there is no easy trick. Life is complicated ;-)

navidshaikh

We should re-group the traffic and tags column next to SERVICE for eg:

➜  client git:(df045ee0) ✗ ./kn revision list
NAME           SERVICE   TRAFFIC   TAGS    GENERATION   AGE   CONDITIONS   READY   REASON
test-wrvjj-1   test      100%      v1,v2   1            31m   3 OK / 4     True

navidshaikh · 2019-12-23T10:16:35Z

pkg/kn/commands/revision/list.go

+
+				traffic, tags := trafficForRevision(revision.Name, service)
+				revision.Annotations[RevisionTrafficAnnotation] = fmt.Sprintf("%d%%", traffic)
+				revision.Annotations[RevisionTagsAnnotation] = strings.Join(tags, ",")


Can we avoid modifying the revision here by

+1

We can use trafficForRevision in printRevision to fetch the traffic and tags.

knative-metrics-robot · 2019-12-23T23:44:25Z

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/revision/human_readable_flags.go	95.0%	95.5%	0.5
pkg/kn/commands/revision/list.go	85.2%	86.2%	1.0

maximilien · 2019-12-24T22:22:29Z

We should re-group the traffic and tags column next to SERVICE for eg:

➜  client git:(df045ee0) ✗ ./kn revision list
NAME           SERVICE   TRAFFIC   TAGS    GENERATION   AGE   CONDITIONS   READY   REASON
test-wrvjj-1   test      100%      v1,v2   1            31m   3 OK / 4     True

Done

maximilien · 2020-01-07T00:35:46Z

@navidshaikh this is ready to merge IMO.

rhuss

See my comment. As it seems that we disagree (see last comment) here on the implementation, let's merge it and let me make an alternative proposal (which might also illustrate my point a bit better).

/lgtm

knative-prow-robot · 2020-01-10T09:15:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, rhuss

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,rhuss]

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

rhuss · 2020-01-10T10:25:31Z

Looks like a flaky error

customresourcedefinition.apiextensions.k8s.io/services.serving.knative.dev created
error: unable to recognize "https://storage.googleapis.com/knative-nightly/serving/latest/serving.yaml": no matches for kind "Image" in version "caching.internal.knative.dev/v1alpha1"
Installing the rest of serving components from https://storage.googleapis.com/knative-nightly/serving/latest/serving.yaml
customresourcedefinition.apiextensions.k8s.io/images.caching.internal.knative.dev unchanged

/retest

navidshaikh · 2020-01-10T11:19:24Z

/retest

timeout: service 'hello' not ready after 600 seconds

rhuss · 2020-01-10T11:53:35Z

/retest

rhuss · 2020-01-10T17:09:29Z

pkg/kn/commands/revision/list.go

+			var service *v1alpha1.Service
+			var serviceName string
+			for _, revision := range revisionList.Items {
+				if serviceName == "" || revision.Labels[serving.ServiceLabelKey] != serviceName {


(just working on an followup PR, but wanna leave some extra comments): It is by no means guaranteed that revisions from the list are sorted according to service. Better store in a map for latter referral.

Please don't fix I'm on it.

Co-authored-by: Yaakov Selkowitz <[email protected]>

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

knative-prow-robot requested review from cppforlife and sixolet December 20, 2019 02:17

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 20, 2019

rhuss requested changes Dec 20, 2019

View reviewed changes

maximilien force-pushed the issue580 branch 2 times, most recently from 102541c to df045ee Compare December 20, 2019 20:41

navidshaikh reviewed Dec 23, 2019

View reviewed changes

feature(knative#580): adds traffic and tags information to revision list

6e6eadc

maximilien force-pushed the issue580 branch from df045ee to 6e6eadc Compare December 23, 2019 23:42

rhuss approved these changes Jan 10, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jan 10, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2020

knative-prow-robot merged commit ac2170c into knative:master Jan 10, 2020

rhuss reviewed Jan 10, 2020

View reviewed changes

maximilien deleted the issue580 branch January 10, 2020 21:43

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

remove label and close issue if failed creating issue (knative#581)

243f1c0

dsimansk pushed a commit to dsimansk/client that referenced this pull request Jan 21, 2021

Package and serve binaries for all RHEL architectures (knative#581)

28dee37

Co-authored-by: Yaakov Selkowitz <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(#580): adds traffic and tags information to revision list #581

feature(#580): adds traffic and tags information to revision list #581

maximilien commented Dec 20, 2019 •

edited

Loading

maximilien commented Dec 20, 2019

daisy-ycguo commented Dec 20, 2019

rhuss left a comment

rhuss Dec 20, 2019

maximilien Dec 20, 2019

navidshaikh Dec 23, 2019

maximilien Dec 23, 2019

navidshaikh Dec 31, 2019 •

edited

Loading

maximilien Jan 9, 2020

maximilien Jan 9, 2020

rhuss Jan 10, 2020

maximilien Jan 10, 2020

rhuss Jan 10, 2020

navidshaikh left a comment

navidshaikh Dec 23, 2019

knative-metrics-robot commented Dec 23, 2019

maximilien commented Dec 24, 2019

maximilien commented Jan 7, 2020

rhuss left a comment

knative-prow-robot commented Jan 10, 2020

rhuss commented Jan 10, 2020 •

edited

Loading

navidshaikh commented Jan 10, 2020

rhuss commented Jan 10, 2020

rhuss Jan 10, 2020

feature(#580): adds traffic and tags information to revision list #581

feature(#580): adds traffic and tags information to revision list #581

Conversation

maximilien commented Dec 20, 2019 • edited Loading

Proposed Changes

maximilien commented Dec 20, 2019

daisy-ycguo commented Dec 20, 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

navidshaikh Dec 31, 2019 • edited Loading

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

navidshaikh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Dec 23, 2019

maximilien commented Dec 24, 2019

maximilien commented Jan 7, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 10, 2020

rhuss commented Jan 10, 2020 • edited Loading

navidshaikh commented Jan 10, 2020

rhuss commented Jan 10, 2020

Choose a reason for hiding this comment

maximilien commented Dec 20, 2019 •

edited

Loading

navidshaikh Dec 31, 2019 •

edited

Loading

rhuss commented Jan 10, 2020 •

edited

Loading