Skip to content

examples(nodejs): bump to the latest 3.x version of cloudevents SDK #2769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 7, 2020

Conversation

lance
Copy link
Member

@lance lance commented Aug 20, 2020

A few changes to the API were made in the recent release of the SDK. This commit updates the dependency and modifies the source to work with the new API.

Proposed Changes

  • Update the Node.js example to use the cloudevents module version 3.1.0
  • Modify the example code to work with the changed API
  • Return the actual CloudEvent instance in the response body in receiveAndReply()

A few changes to the API were made in the recent release of the SDK.
This commit updates the dependency and modifies the source to work
with the new API.
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 20, 2020
@markusthoemmes
Copy link
Contributor

I guess we have no test automation here and i'll have to manually check this is sound? 😅

@markusthoemmes
Copy link
Contributor

Actually @csantanapr would you mind giving this a look? You're more knowledgeable on this front than I am.

The changes look good on the surface though, thanks @lance!

@lance
Copy link
Member Author

lance commented Sep 10, 2020

@markusthoemmes new release of the module is landing soon. Maybe this should hold and I can update the PR when that is published.

@csantanapr
Copy link
Member

csantanapr commented Sep 10, 2020

Thanks for the ping @markusthoemmes

@lance next time just open an issue I'm happy to update the sample or just assigned me as reviewer to your PR.

I'll give the code a try.

@lance how "soon" is the new update landing and how it changes your PR?

We can always merge this PR and do another when the updates to the SDK lands.

@lance
Copy link
Member Author

lance commented Sep 10, 2020

@csantanapr there are 3.2.1 and 4.0.0 releases imminent - should land this week or early next at the latest. In 4.0.0 there are some major interface changes that affect how a user receives CE messagaes. If you want to land this PR now, that's not a problem.

@csantanapr
Copy link
Member

@lance oh I thought it was more like a 3.3 bump not a major API release.

With 2.x 3.x and 4.x release so close together it looks like the SDK is having a lot of fun turns.

I would preferred end users just ignore and not be aware of 3.x and focus on 4.x. The more API changes the less trust on the library.

@abrennan89 abrennan89 added the triage/needs-eng-input Engineering input is requested label Nov 3, 2020
@abrennan89 abrennan89 modified the milestones: v0.20.0, v0.19.0 Nov 3, 2020
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 3, 2020
@lance
Copy link
Member Author

lance commented Nov 3, 2020

@csantanapr I went ahead and just bumped to 3.2.0. The 4.x version is still pending release.

@csantanapr
Copy link
Member

I will review it @lance with 3.x then

@@ -15,11 +15,10 @@ limitations under the License.
*/

const express = require('express')
const { CloudEvent, HTTPEmitter, HTTPReceiver } = require('cloudevents-sdk')
const { CloudEvent, Emitter, HTTP, headersFor } = require('cloudevents')
Copy link
Member

Choose a reason for hiding this comment

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

Small nit headersFor is an unused variable

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@csantanapr
Copy link
Member

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2020
@csantanapr
Copy link
Member

/approve

@csantanapr
Copy link
Member

@lance it was a good thing base image has arm64 cc @mattmoor

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2020
@csantanapr
Copy link
Member

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2020
@csantanapr
Copy link
Member

/approve

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csantanapr, lance

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

@knative-prow-robot knative-prow-robot merged commit 8323b64 into knative:master Nov 7, 2020
RichieEscarez pushed a commit to RichieEscarez/docs that referenced this pull request Mar 6, 2021
…native#2769)

* examples(nodejs): bump to the latest 3.x version of cloudevents SDK

A few changes to the API were made in the recent release of the SDK.
This commit updates the dependency and modifies the source to work
with the new API.

* squash: bump cloudevents to 3.2.0

Signed-off-by: Lance Ball <[email protected]>

* fixup: incorporate PR feedback

Signed-off-by: Lance Ball <[email protected]>
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/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/needs-eng-input Engineering input is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants