fix: [backport] support mTLS in 1.0 Binary and Structured emitters #106

lance · 2020-04-30T20:53:32Z

This commit modifies both of the 1.0 emitters so that they may
accept typed objects as a part of the configuration. When using
mTLS in Node, you need to provide an Agent to the underlying
HTTP handler. In this case, Axios will pass this object along to
Node.js when it is provided.

Landed in: 3a063d7
Pull Request: #53

Fixes: #48

Signed-off-by: Lance Ball lball@redhat.com

This commit modifies both of the 1.0 emitters so that they may accept typed objects as a part of the configuration. When using mTLS in Node, you need to provide an `Agent` to the underlying HTTP handler. In this case, Axios will pass this object along to Node.js when it is provided. Fixes: cloudevents#48 Signed-off-by: Lance Ball <lball@redhat.com>

lance · 2020-04-30T21:06:49Z

The Codacy failure is due to this problem:

Seems like the v1.x.y is not being analyzed. To review this pull request enable the branch in the repository settings.

grant

Looks fine, however we should use ES6 JS features. I gave some examples.

grant · 2020-04-30T21:05:09Z

lib/bindings/http/emitter_structured.js

+  return axios.request(this.config).then(response => {
+    delete this.config[Constants.DATA_ATTRIBUTE];
+    return response;
+  });


This is very confusing.

Why don't we just do this?

const res = await axios.request(this.config); delete this.config[Constants.DATA_ATTRIBUTE]; return res;

The outer function needs to be async of course.

The change you suggest introduces a race condition.

No, where's the race? That's not how Node executes.

I vote for then notation.

@grant try this in your browser https://gist.github.com/lance/ff1bf6d2da191ad164257349486c4c95

Although, playing around with this code reveals to me that the race condition exists even using then(). Boo.

grant · 2020-04-30T21:10:10Z

lib/bindings/http/emitter_structured.js

@@ -1,30 +1,23 @@
 var axios = require("axios");

 const Constants = require("./constants.js");
+const defaults = {};


Why are we creating an empty object and then adding properties to it?

const defaults = { [Constants.HEADERS]: { [Constants.HEADER_CONTENT_TYPE]: Constants.DEFAULT_CE_CONTENT_TYPE } };

You introduced new syntax to me. I wasn't aware that you could bracket a nested property as a field name.

OK. This is a feature of JavaScript ES2015. Here's a reference:
https://stackoverflow.com/a/19837961

I did not know either! Looks good

grant · 2020-04-30T21:11:58Z

lib/bindings/http/emitter_structured.js

-    this.config[Constants.HEADERS][Constants.HEADER_CONTENT_TYPE] =
-      Constants.DEFAULT_CE_CONTENT_TYPE;
-  }
+  this.config = Object.assign({}, defaults, configuration);


Why are we using Object.assign when we can just use the native JavaScript object spread operator?

this.config = {...defaults, ...configuration};

The commit happened before we dropped support for early versions of Node.js so this would not have passed CI. It's a reasonable change for master now that we've dropped 6 and 8.

I'm not sure of the CI system yet but Node versions < 10 were end of life Dec 31, 2019. Completely understandable, but we should only use non-deprecated versions of Node in our CI.

@grant yes, I am aware. That's why I updated CI last month to only use 10 & 12. aa2cef6#diff-354f30a63fb0907d4ad57269548329e3

@lance I've created this #119 but I don't think a new PR is necessary maybe if you and the others agree with that practice you can update your PR instead wdyt ?

@helio-frota I think submitting a separate PR for that is fine.

grant · 2020-04-30T21:20:53Z

test/http_binding_1.js

+          key: 'other value'
+        })
+      })
+      return event.emit(cloudevent).then(response => {


This can also be converted to ES6.

Again - this was prior to dropping Node 6 and 8 support.

grant · 2020-04-30T21:21:46Z

lib/bindings/http/emitter_binary.js

+  config[Constants.DATA_ATTRIBUTE] = data;
+  config.headers = headers;
+
+    // Return the Promise


Formatting.

lance · 2020-04-30T21:26:50Z

@grant this is a backport of an existing commit, cherry picked onto the v1.x.y branch. I think it should remain as the original commit. The whole entire idea is that this 1.x branch get the exact commits that landed on master. If you want to change syntax to ES6 on master that's great. We can chose to backport that or not.

grant · 2020-04-30T23:07:07Z

@grant this is a backport of an existing commit, cherry picked onto the v1.x.y branch. I think it should remain as the original commit. The whole entire idea is that this 1.x branch get the exact commits that landed on master. If you want to change syntax to ES6 on master that's great. We can chose to backport that or not.

OK. So we're basically just freezing a version of master and setting it to this branch?

In that case, that's fine, and we should indicate the PR is meant for that. We probably don't need a review if this is just mirroring master.

lance · 2020-05-01T13:00:09Z

OK. So we're basically just freezing a version of master and setting it to this branch?

Sort of. Here I used git cherry-pick to apply a single commit from master to the v1.x.y branch. This results in the bug fix applied in the commit on master being backported to the v1.x.y branch. The idea is that we don't want all of the commits from master landing in v1.1.0 or any other subsequent release of the 1.x line. We just want the commits that are important. No new features, no breaking changes. Just bug fixes, etc.

I understand that there aren't a lot of people using this module right now. But, there are some people using it, I'm sure. And those people are all on 1.0.0 right now. If our 2.0.0 release is going to have major breaking changes (say the API is completely changed - not using builder patter for example), we can't expect people to immediately rewrite their code to accommodate our new release. HOWEVER, they still should have the bug fixes and updates that keep their application working correctly through patch/minor version level releases of the 1.x line. That's what this whole branch/backport thing is about.

lance · 2020-05-01T13:04:44Z

In that case, that's fine, and we should indicate the PR is meant for that. We probably don't need a review if this is just mirroring master.

The v1.x.y-backport label is the indicator. As well as the two lines in the PR description,

Landed in: 3a063d7
Pull Request: #53

"Landed in: xxxxx" indicates what commit this cherry pick originally landed in on master.
"Pull Request: #53" indicates what pull request had the original fix.

Backporting isn't always easy and doesn't always apply cleanly. It's not as simple as "mirroring master" - it's taking a single commit from the master branch and applying it to the backport branch. As you are aware, the velocity of change in the repo has increased quite a bit. It's easy to get conflicts when backporting and you need to take care with what you are doing. I think it's important to still have approvals for these kinds of PRs. They shouldn't just be allowed to land without review.

fabiojose · 2020-05-03T17:51:24Z

v1/index.js

@@ -22,5 +22,7 @@ module.exports = {
  BinaryHTTPEmitter,
  StructuredHTTPReceiver,
  BinaryHTTPReceiver,
+  Cloudevent: event,


fabiojose

LGTM

grant

Merging into cloudevents:v1.x.y looks fine.

lance requested review from grant and fabiojose April 30, 2020 20:53

lance added the version/1.x.backport Changes that should be backported to the 1.x release line label Apr 30, 2020

lance mentioned this pull request Apr 30, 2020

docs: add instructions and details to contributors guide #105

Merged

grant suggested changes Apr 30, 2020

View reviewed changes

fabiojose reviewed May 3, 2020

View reviewed changes

fabiojose approved these changes May 3, 2020

View reviewed changes

lance changed the title ~~fix: support mTLS in 1.0 Binary and Structured emitters~~ fix: [backport] support mTLS in 1.0 Binary and Structured emitters May 4, 2020

helio-frota mentioned this pull request May 4, 2020

Suggestion of Node.js versions on CI based on package maintenance group practices #119

Closed

grant approved these changes May 4, 2020

View reviewed changes

helio-frota self-requested a review May 4, 2020 19:39

helio-frota approved these changes May 4, 2020

View reviewed changes

lance merged commit 0cd9256 into cloudevents:v1.x.y May 5, 2020

lance deleted the v1.x-backport branch May 5, 2020 21:45

fix: [backport] support mTLS in 1.0 Binary and Structured emitters #106

fix: [backport] support mTLS in 1.0 Binary and Structured emitters #106

Uh oh!

Conversation

lance commented Apr 30, 2020

Uh oh!

lance commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lance Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

lance commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grant commented Apr 30, 2020

Uh oh!

lance commented May 1, 2020

Uh oh!

lance commented May 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiojose left a comment

Choose a reason for hiding this comment

Uh oh!

grant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lance commented Apr 30, 2020 •

edited

Loading

lance Apr 30, 2020 •

edited

Loading

lance commented Apr 30, 2020 •

edited

Loading