Add sample for cloudevents-nodejs by csantanapr · Pull Request #2446 · knative/docs

csantanapr · 2020-05-12T03:24:48Z

Signed-off-by: Carlos Santana csantana@us.ibm.com

Fixes #2445

cc @lance @mattmoor

mattmoor

I'm no node expert, but a couple of drive-by comments.

Thanks for putting this together! 🎉

lance

So glad to see some of the upcoming API in use.

lance

"files.insertFinalNewline": true in VSCode config should enable auto insert of new line if one doesn’t exist. ;)

csantanapr · 2020-05-13T04:03:43Z

@mattmoor @lance let me know if this is good for now to merge this PR, I can do another update once the sdk 2.0 is out.

I think I'm done with the last changes, looks good that I can use as starting point to build a cool demo on top using brokers and triggers

vaikas

Awesome!!! Just couple of nits I think? Thanks!!

csantanapr · 2020-05-13T19:19:54Z

thanks @vaikas for the quick review, I think I addressed all your comments

vaikas · 2020-05-13T19:21:21Z

/lgtm
/hold
Holding since other folks had comments also, but thanks for doing this!!!

csantanapr · 2020-05-13T19:25:26Z

Ah its /hold I was looking for a way to put the PR in Draft for wip with /draft or /wip

lance · 2020-05-13T19:47:53Z

@csantanapr LGTM

vaikas · 2020-05-19T22:11:15Z

/unhold

vaikas · 2020-05-19T22:11:29Z

/approve

mattmoor · 2020-05-20T02:05:43Z

@csantanapr looks like you need another file, see the other PR that landed recently for the right way:

index_test.go:81: Could not verify docs: README.md missing bundle (you need an 'index.md' or '_index.md') in "docs/serving/samples/cloudevents/cloudevents-nodejs"

mattmoor · 2020-05-20T02:31:25Z

/unhold

I think the two things this needs are:

the license header
the index.md thing from the test failure

Otherwise I tried it with and without K_SINK and it worked great 🎉

mattmoor · 2020-05-20T02:42:36Z

We also need to update the table here: https://github.com/knative/docs/blob/master/docs/serving/samples/README.md

csantanapr · 2020-05-20T13:27:56Z

/hold

Thanks to all for reviewing the PR

I saw the other day from Evan on the refactor for the index.md and the testing for consistently +1

I will rebase and squash and update.

I was thinking on looking on adding functional e2e similar to the hello samples to avoid PITA manual testing but that would be a different PR

What you folks think on adding typescript version or Deno? Maybe one sample to cover both Deno and typescript ?

mattmoor · 2020-05-20T14:30:58Z

@csantanapr Ha! I was just asking about mirroring that test setup in #test now that these are expanding to more languages. I agree about doing that separately, and if you'd be willing to help drive that it'd be awesome!

csantanapr · 2020-05-20T14:46:49Z

@mattmoor yes I will also work on e2e tests good opportunity for me stop procrastinating and getting my hands dirty with some Go

Signed-off-by: Carlos Santana <csantana@us.ibm.com>

csantanapr · 2020-05-21T02:17:43Z

/unhold

csantanapr · 2020-05-21T02:28:34Z

@mattmoor or @vaikas can you give this PR another look.
I tried to address all the feedback

mattmoor · 2020-05-21T19:00:27Z

/approve

Going to approve so the next push stages it :)

Signed-off-by: Carlos Santana <csantana@us.ibm.com>

csantanapr · 2020-05-21T19:12:44Z

thanks @mattmoor I added license headers

csantanapr · 2020-05-21T19:13:07Z

And no # Foo to be found 😅

lance

cloudevents/sdk-javascript#178

Signed-off-by: Carlos Santana <csantana@us.ibm.com>

csantanapr · 2020-05-22T18:28:22Z

@mattmoor bump

mattmoor

/lgtm
/approve

knative-prow-robot · 2020-05-22T18:30:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csantanapr, mattmoor, vaikas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mattmoor,vaikas]

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 12, 2020

knative-prow-robot requested review from dprotaso and mattmoor May 12, 2020 03:24

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

mattmoor reviewed May 12, 2020

View reviewed changes

csantanapr commented May 12, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/Dockerfile Resolved

csantanapr commented May 12, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/Dockerfile Resolved

csantanapr commented May 12, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/service.yaml Resolved

csantanapr commented May 12, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/index.js Resolved

mattmoor reviewed May 12, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/index.js Resolved

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/index.js Resolved

lance reviewed May 12, 2020

View reviewed changes

mattmoor reviewed May 12, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/package.json Resolved

csantanapr commented May 12, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/package.json Resolved

lance reviewed May 13, 2020

View reviewed changes

vaikas reviewed May 13, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2020

knative-prow-robot assigned vaikas May 13, 2020

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

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 19, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2020

lance reviewed May 20, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/index.js Resolved

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2020

csantanapr force-pushed the issuexx-add-cloudevent-sample-nodejs branch from d960219 to 3e08667 Compare May 20, 2020 22:43

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 20, 2020

Add sample for cloudevents-nodejs

96523ee

Signed-off-by: Carlos Santana <csantana@us.ibm.com>

csantanapr force-pushed the issuexx-add-cloudevent-sample-nodejs branch from f5eaa28 to 96523ee Compare May 21, 2020 02:16

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2020

mattmoor reviewed May 21, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/README.md Resolved

Add license headers

c03a5f1

Signed-off-by: Carlos Santana <csantana@us.ibm.com>

csantanapr commented May 21, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/index.js Resolved

lance reviewed May 21, 2020

View reviewed changes

Comment thread docs/serving/samples/cloudevents/cloudevents-nodejs/index.js Resolved

better to return 202 Accepted

fb3eff1

Signed-off-by: Carlos Santana <csantana@us.ibm.com>

mattmoor reviewed May 22, 2020

View reviewed changes

knative-prow-robot assigned mattmoor May 22, 2020

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

knative-prow-robot merged commit 7c8c382 into knative:master May 22, 2020

Conversation

csantanapr commented May 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lance left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lance left a comment

Choose a reason for hiding this comment

Uh oh!

csantanapr commented May 13, 2020

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

csantanapr commented May 13, 2020

Uh oh!

vaikas commented May 13, 2020

Uh oh!

csantanapr commented May 13, 2020

Uh oh!

lance commented May 13, 2020

Uh oh!

vaikas commented May 19, 2020

Uh oh!

vaikas commented May 19, 2020

Uh oh!

mattmoor commented May 20, 2020

Uh oh!

mattmoor commented May 20, 2020

Uh oh!

mattmoor commented May 20, 2020

Uh oh!

Uh oh!

csantanapr commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattmoor commented May 20, 2020

Uh oh!

csantanapr commented May 20, 2020

Uh oh!

csantanapr commented May 21, 2020

Uh oh!

csantanapr commented May 21, 2020

Uh oh!

Uh oh!

mattmoor commented May 21, 2020

Uh oh!

csantanapr commented May 21, 2020

Uh oh!

csantanapr commented May 21, 2020

Uh oh!

Uh oh!

lance left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

csantanapr commented May 22, 2020

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

csantanapr commented May 20, 2020 •

edited

Loading