Skip to content

Conversation

@aliok
Copy link
Member

@aliok aliok commented May 22, 2020

Fixes #issue-number or description of the problem the PR solves

Proposed Changes

  • Add a tutorial for writing an event source easy way
  • Tutorial is done with Javascript
  • Some references are pointing at personal repos/images as they're not published yet or they're not part of Knative GH org yet

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 22, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 22, 2020
@aliok
Copy link
Member Author

aliok commented May 22, 2020

/retest

template:
spec:
containers:
- image: docker.io/aliok/event_display-864884f202126ec3150c5fcef437d90c@sha256:93cb4dcda8fee80a1f68662ae6bf20301471b046ede628f3c3f94f39752fbe08
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment there's a bug in the latest published image of gcr.io/knative-releases/knative.dev/eventing-contrib/cmd/event_display that it doesn't display events properly.

The bug is fixed but image is not published yet. Thus, I reference my own image here. This is to be updated later.

@slinkydeveloper maybe you can add a link to the bug?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we have a link for that bug, but it was fixed between sdk-go RC1 and RC2

@aliok
Copy link
Member Author

aliok commented May 22, 2020

cc @abrennan89 if you want to have a look

@aliok
Copy link
Member Author

aliok commented May 22, 2020

/test pull-knative-docs-go-coverage

@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented May 22, 2020

@aliok: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-knative-docs-go-coverage 87d1f91 link /test pull-knative-docs-go-coverage

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this writeup!

/approve

/assign @n3wscott

I think Scott should probably finish the review (and this might get him off the hook on #2492).


# Install app dependencies
# A wildcard is used to ensure both package.json AND package-lock.json are copied
# where available (npm@5+)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're using node:10, this would be later than npm@5, right?

Copy link
Member Author

@aliok aliok May 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted in #2512

As stated in [tutorial on writing a Source with a Receive Adapter](../writing-receive-adapter-source/README.md), there are multiple ways to
create event sources. The way in that tutorial is to create an independent event source that has its own CRD.

In this tutorial though, you will build an event source in Javascript and use it with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In this tutorial though, you will build an event source in Javascript and use it with
This tutorial provides a simpler mechanism to build an event source in Javascript and use it with

[ContainerSource](../../../eventing/sources/README.md#meta-sources) and / or [SinkBinding](../../../eventing/sources/README.md#meta-sources).

[ContainerSource](../../../eventing/sources/README.md#meta-sources) is an easy way to turn any dispatcher container into an Event Source.
Similarly, another option is using [SinkBinding](../../../eventing/sources/README.md#meta-sources)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were recommending SinkBinding over ContainerSource at this point?

@n3wscott

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a sentence about the preferred way in #2512

Similarly, another option is using [SinkBinding](../../../eventing/sources/README.md#meta-sources)
which provides a framework for injecting environment variables into any Kubernetes resource which has a `spec.template` that looks like a Pod (aka PodSpecable).

Code for this tutorial is available [here](./).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this link will work correctly on the website; but you can put in git clone instructions.

The sink URL to post the events will be made available to the application via the `K_SINK` environment variable by `ContainerSource`.

```javascript
// File - index.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to use the {{% readfile file="index.js" %}} shortcode here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do that later if necessary. The code I embedded in the tutorial is a bit shorter and more focused than the code in the files.

}, 1000);
```

```dockerfile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Ditto)

docker push path/to/image/registry/node-knative-heartbeat-source:v1
```

Create the event display service which simply logs any cloudevents posted to it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to use kn to create this service, instead of needing to fill in YAML?

Actually, that may not be necessary if this is always a fixed (published) image. Is event-display part of the released container images?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event-display is part of the released container images, yep.


Check the logs of the event display service. You will see a new message is pushed every second:
```bash
$ kubectl logs event-display-dpplv-deployment-67c9949cf9-bvjvk -c user-container
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$ kubectl logs event-display-dpplv-deployment-67c9949cf9-bvjvk -c user-container
$ kubectl logs -l serving.knative.dev/service=event-display -c user-container

This avoids need to look up the name of the pods before reading the logs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

}
```

If you are interested in to see what is injected into the event source, check the logs of it:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you are interested in to see what is injected into the event source, check the logs of it:
If you are interested in to see what is injected into the event source, you can check the logs:

Not sure if you want to have the code dump out all the environment variables just for pedagogical purposes.

- It is faster in terms of serialization and deserialization
- It works better with cloudevents-aware proxies (like Knative Channels) can simply check the header instead of parsing the payload

## Making use of SinkBinding
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're recommending SinkBinding going forward, so I'd move this above ContainerSource (or maybe prefer to just document SinkBinding).

@n3wscott to verify

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, evankanderson

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:

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

Copy link
Contributor

@n3wscott n3wscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Thanks for the sample and write-up. Showing that sink binding and container source are interchangable at the container level is exactly the message we want to be telling.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2020
@knative-prow-robot knative-prow-robot merged commit 3484cdb into knative:master May 23, 2020
@aliok
Copy link
Member Author

aliok commented May 27, 2020

Created #2512 for the comments that I couldn't address before the PR is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants