message_dumper to event_display #1126

samodell · 2019-04-01T23:35:46Z

Fixes #1110

knative-prow-robot · 2019-04-01T23:35:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: samodell

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

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

samodell

Added a couple questions about this update. PTAL, @n3wscott and @Harwayne

samodell · 2019-04-01T23:36:23Z

docs/eventing/debugging/example.yaml

-          # This corresponds to
-          # https://github.com/knative/eventing-sources/blob/v0.2.1/cmd/message_dumper/dumper.go.
-          image: gcr.io/knative-releases/github.com/knative/eventing-sources/cmd/message_dumper@sha256:ab5391755f11a5821e7263686564b3c3cd5348522f5b31509963afb269ddcd63
+          image: gcr.io/knative-releases/github.com/knative/eventing-sources/cmd/event_display@sha256:ab5391755f11a5821e7263686564b3c3cd5348522f5b31509963afb269ddcd63


I didn't update everything after @sha; does that need to be updated too?

yes definitely needs to be updated

samodell · 2019-04-01T23:37:39Z

docs/eventing/samples/cronjob-source/README.md

 spec:
  runLatest:
    configuration:
      revisionTemplate:


Should the name above (on line 20) be updated from message-dumper to something like event-display? @Harwayne Would that impact the rest of the code for the samples?

Yes. service.yaml will need to change in the same way.

Harwayne · 2019-04-02T19:46:27Z

docs/eventing/debugging/example.yaml

-          # This corresponds to
-          # https://github.com/knative/eventing-sources/blob/v0.2.1/cmd/message_dumper/dumper.go.
-          image: gcr.io/knative-releases/github.com/knative/eventing-sources/cmd/message_dumper@sha256:ab5391755f11a5821e7263686564b3c3cd5348522f5b31509963afb269ddcd63
+          image: gcr.io/knative-releases/github.com/knative/eventing-sources/cmd/event_display@sha256:ab5391755f11a5821e7263686564b3c3cd5348522f5b31509963afb269ddcd63


Harwayne · 2019-04-02T19:47:31Z

docs/eventing/debugging/example.yaml

-          # This corresponds to
-          # https://github.com/knative/eventing-sources/blob/v0.2.1/cmd/message_dumper/dumper.go.
-          image: gcr.io/knative-releases/github.com/knative/eventing-sources/cmd/message_dumper@sha256:ab5391755f11a5821e7263686564b3c3cd5348522f5b31509963afb269ddcd63
+          image: gcr.io/knative-releases/github.com/knative/eventing-sources/cmd/event_display@sha256:ab5391755f11a5821e7263686564b3c3cd5348522f5b31509963afb269ddcd63


I wouldn't change the debugging example. If you do, the MD also needs to change (not purely mechanical changes either).

Harwayne · 2019-04-02T19:48:38Z

docs/eventing/samples/cronjob-source/README.md

 spec:
  runLatest:
    configuration:
      revisionTemplate:


Yes. service.yaml will need to change in the same way.

Harwayne · 2019-04-02T19:50:29Z

docs/eventing/samples/gcp-pubsub-source/trigger.yaml

-            # This corresponds to
-            # https://github.com/knative/eventing-sources/blob/release-0.3/cmd/message_dumper/dumper.go
-            image: gcr.io/knative-releases/github.com/knative/eventing-sources/cmd/message_dumper@sha256:8423232db7a7b4010c0cfbfaef95745efe962631af9b7456903825801a7893f7
+            image: gcr.io/knative-releases/github.com/knative/eventing-sources/cmd/event_display@sha256:8423232db7a7b4010c0cfbfaef95745efe962631af9b7456903825801a7893f7


SHA definitely needs to change. Probably wait until 0.5.0 has been cut, then use that. I recommend leaving a comment describing where that image comes from (e.g. 'This corresponds to https://github.com/knative/eventing-sources/blob/v0.5.0/cmd/event_display/main.go.')

Harwayne · 2019-04-05T23:03:04Z

docs/eventing/samples/cronjob-source/README.md

+    name: event-display
 ```

 Use following command to create the event source from `cronjob-source.yaml`:


I can't seem to comment on line 69.

The 'Verify' command should be:

kubectl logs -l serving.knative.dev/service=event-display -c user-container --since=10m

The output text will also be a bit different, but I don't know exactly what it will look like right now. We can probably leave the existing text for the time being.

Harwayne · 2019-04-05T23:04:40Z

docs/eventing/samples/gcp-pubsub-source/trigger.yaml

      apiVersion: serving.knative.dev/v1alpha1
      kind: Service
-      name: message-dumper
+      name: event-display


Similar to the previous comment, the readme needs an updated verification section. I think s/message-dumper/event-display/g would work.

Harwayne · 2019-04-05T23:06:22Z

docs/eventing/samples/github-source/README.md

+            image: gcr.io/knative-releases/github.com/knative/eventing-sources/cmd/event_display
 ```

 Enter the following command to create the service from `service.yaml`:


Similar comment on line 150.

I would replace the entire Create Events command with:

kubectl logs -l serving.knative.dev/service=event-display -c user-container --since=10m

Harwayne · 2019-04-05T23:07:39Z

docs/eventing/samples/iot-core/trigger.yaml

 kind: Service
 metadata:
-  name: message-dumper
+  name: event-display


Once again the readme will have to be updated. s/message-dumper/event-display/g.

Harwayne · 2019-04-05T23:07:44Z

docs/eventing/samples/kubernetes-event-source/trigger.yaml

 kind: Service
 metadata:
-  name: message-dumper
+  name: event-display


Once again the readme will have to be updated. s/message-dumper/event-display/g.

I think I have that done globally.

Harwayne

/lgtm

samodell added this to the v0.5 Docs milestone Apr 1, 2019

samodell requested review from Harwayne and n3wscott April 1, 2019 23:35

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

knative-prow-robot added approved size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 1, 2019

knative-prow-robot requested review from carieshmarie and gyliu513 April 1, 2019 23:35

samodell commented Apr 1, 2019

View reviewed changes

samodell removed the approved label Apr 1, 2019

Harwayne reviewed Apr 2, 2019

View reviewed changes

samodell force-pushed the msgdumper branch from f130230 to 06b97ba Compare April 5, 2019 21:59

knative-prow-robot added the approved label Apr 5, 2019

samodell force-pushed the msgdumper branch 2 times, most recently from f5281a5 to 4a39b75 Compare April 5, 2019 22:49

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 5, 2019

samodell force-pushed the msgdumper branch from 4a39b75 to caa8dbb Compare April 5, 2019 22:51

Harwayne reviewed Apr 5, 2019

View reviewed changes

message_dumper to event_display

afeca56

samodell force-pushed the msgdumper branch from caa8dbb to afeca56 Compare April 5, 2019 23:21

Harwayne reviewed Apr 5, 2019

View reviewed changes

knative-prow-robot assigned Harwayne Apr 5, 2019

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

knative-prow-robot merged commit d5735fd into knative:master Apr 5, 2019

message_dumper to event_display #1126

message_dumper to event_display #1126

Uh oh!

Conversation

samodell commented Apr 1, 2019

Uh oh!

knative-prow-robot commented Apr 1, 2019

Uh oh!

samodell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants