Feature/plugins v3spec part1 connect #73 #381

DrMegavolt · 2017-08-23T15:54:10Z

Final v1.0 version

codecov · 2017-08-23T15:54:25Z

Codecov Report

Merging #381 into master will decrease coverage by 0.17%.
The diff coverage is 84.56%.

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
- Coverage   91.46%   91.28%   -0.18%     
==========================================
  Files         197      204       +7     
  Lines        8349     8521     +172     
==========================================
+ Hits         7636     7778     +142     
- Misses        713      743      +30

Impacted Files	Coverage Δ
lib/gateway/index.js	`100% <100%> (ø)`	⬆️
lib/index.js	`100% <100%> (ø)`	⬆️
test/plugins/condition.test.js	`100% <100%> (ø)`
test/plugins/gateway-ext.test.js	`100% <100%> (ø)`
test/routing/path-general.test.js	`100% <100%> (+4.54%)`	⬆️
lib/eventBus.js	`100% <100%> (ø)`
test/common/routing.helper.js	`96.77% <100%> (-1.62%)`	⬇️
lib/gateway/pipelines.js	`92.68% <100%> (+0.43%)`	⬆️
test/common/server-helper.js	`96.55% <100%> (ø)`	⬆️
test/plugins/policy.test.js	`100% <100%> (ø)`
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc1fc8e...23f3a79. Read the comment docs.

kevinswiber · 2017-08-29T00:40:57Z

lib/plugins.js

+    this.logger = logger;
+    this.settings = settings || {};
+    this.config = config;
+    this.policies = [];


Instead of exposing the underlying data structure, what do you think about registration functions? manifest.registerPolicy instead of manifest.policies.push.

kevinswiber · 2017-08-30T22:19:36Z

lib/plugins.js

+  };
+};
+
+class PluginManifest {


I think the concept of "manifest" is a little strange in this context. I think of a manifest as a file living outside the code that contains metadata about the contents of the project, usually written in a config format like XML, JSON, or INI. I get that it's serving a similar purpose, but I look at this in code and think, "Gee, is this telling me I can actually put this manifest in a separate config file?" This might be a bias based on my past experiences. Not sure. Also, feels a tad more weird when we start assigning event handlers. Just thought I'd toss this out there. Would it make more sense to just call this... Plugin?

I agree that it looks weird to common approach like that
https://developer.chrome.com/extensions/manifest

the thing is that what it passes is not a plugin, because plugin is entire package itself.
It is something like container/ plugin manager / context.

I cannot name it pluginOptions because it will be confused with parameters and settings

kevinswiber · 2017-08-30T22:34:17Z

Just to clarify, this PR doesn't include support for the following extensions:

Custom database entities
Services (or an internal service registry of any kind)
Consumer Management (custom auth methods)

Is that correct?

DrMegavolt · 2017-08-31T07:30:53Z

and no CLI extensions as well

altsang · 2017-09-01T17:33:22Z

lib/config/system.config.yml

@@ -8,6 +8,9 @@ db:
 cli:
  url: http://localhost:9876

+plugins:
+  # eg-example-plugin:


express-gateway-plugin-example would be the package name, the actual plugin name would be example in the system.config.yml ?

for example:
express-gateway-plugin-oauth2 would be the npm module the entry within system.config.yml - oauth2

ok, make sense. and if user needs to install non convention package they will have 'package' word for that?

This section basically acts as a whitelist, yeah?

If so, why do we need a whitelist if the package has already been explicitly installed?

I like concept installed vs enabled. Same idea as behind policies - have many registered(installed), but use only whitelisted(enabled).
It seems to be standard pattern for plugins. Because installation is typically complex process and enable\disable is 1 sec operation.

Atom\VS Code\Sublime all have that. Kong - same idea.

Even ExpressJS - installed middleware, unless it is used in code doesn't exist

Ah, right. So it's not really a "whitelist" of allowed plugins; it's just a signal to enable. We could have discovery logic for finding a package listed here.

Try wrapping the package name in express-gateway-${name}-plugin.

If that fails, try ${name}.

Error.

At this point I've implemented the logic
read settings for ${name}
if contains package -> require('package')
else ensure name is prefixed with express-gateway-plugin- -> require(name)

So if you really want different name, np use package property

Got it. Thanks.

👍 sounds good to me, the plugins: is so that there is a known declaration and exposure to what global params are in place

simliarly in policies: you can have policies installed but unless you declare it, it can't be referenced in a pipeline

kevinswiber · 2017-09-04T19:25:15Z

lib/plugins.js

+  registerAdminExtension (adminExtension) {
+    this.adminExtensions.push(adminExtension);
+  }
+  registerCliExtension (cliExtension) {


For acronyms in JavaScript identifiers, I use all capital letters as the convention, taken from some built-ins like encodeURIComponent. Would you be willing to switch this to registerCLIExtension?

support for admin extensions support for express-gateway extensions like routes and middleware loading plugin conditions plugin: policy test and replacing direct policy registration for some tests with plugin based approach

DrMegavolt · 2017-09-06T11:42:21Z

@altsang @kevinswiber please take a look once again.

The main difference for last commit it is now possible to provide entire config to gateway entity itself. so it will be completely decoupled from config module.

it allows test code without having real config file loaded first
(Still some tests rely on config automatically loaded by gateway, so backward compatibility is preserved - gateway will do config loading if no config object provided)

Another difference is that previously it was a trick to load fake test policies, now it is possible to just use plugins for that. Some tests where refactored with this approach

kevinswiber · 2017-09-06T23:25:56Z

Hmm. So I'm actually running into an issue trying to load plugins. The require call fails. Agh! Trying to figure it out now.

error: [EG:plugins] Failed to load plugin eg-example-plugin Error: Cannot find module 'eg-example-plugin'
    at Function.Module._resolveFilename (module.js:489:15)
    at Function.Module._load (module.js:439:25)
    at Module.require (module.js:517:17)
    at require (internal/module.js:11:18)
    at Object.module.exports.load (/Users/kevin/projects/ExpressGateway/express-gateway/lib/plugins.js:22:20)
    at Main.run (/Users/kevin/projects/ExpressGateway/express-gateway/lib/index.js:23:35)
    at Object.<anonymous> (/private/tmp/asdfasdasd/my-gateway/server.js:7:4)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)

But I can require it just fine in server.js.

kevinswiber · 2017-09-06T23:55:21Z

Just found this. I think we should use it. Looks like it can be an issue for people developing plugins. Let's circumvent it. It fixed the problem for me.

https://www.npmjs.com/package/parent-require

@DrMegavolt

altsang · 2017-09-07T02:15:36Z

Jared's stuff is solid 👍

DrMegavolt · 2017-09-07T11:43:31Z

parent-require is integrated

DrMegavolt self-assigned this Aug 23, 2017

DrMegavolt requested review from kevinswiber and altsang August 23, 2017 15:54

DrMegavolt added the in progress label Aug 23, 2017

DrMegavolt changed the title ~~Feature/plugins v3spec part1~~ Feature/plugins v3spec part1 connect #73 Aug 23, 2017

kevinswiber reviewed Aug 29, 2017

View reviewed changes

kevinswiber reviewed Aug 30, 2017

View reviewed changes

altsang requested changes Sep 1, 2017

View reviewed changes

kevinswiber reviewed Sep 4, 2017

View reviewed changes

DrMegavolt force-pushed the feature/plugins-v3spec-part1 branch 3 times, most recently from 2657d10 to 77232c1 Compare September 6, 2017 10:28

engine to load plugins

20288c9

support for admin extensions support for express-gateway extensions like routes and middleware loading plugin conditions plugin: policy test and replacing direct policy registration for some tests with plugin based approach

DrMegavolt force-pushed the feature/plugins-v3spec-part1 branch from 77232c1 to 20288c9 Compare September 6, 2017 10:30

cleanup

4a7182e

fix package-lock

1288607

kevinswiber approved these changes Sep 6, 2017

View reviewed changes

altsang approved these changes Sep 6, 2017

View reviewed changes

fixed plugin loading issue when running as library

23f3a79

DrMegavolt merged commit b63a379 into master Sep 7, 2017

DrMegavolt removed the in progress label Sep 7, 2017

DrMegavolt deleted the feature/plugins-v3spec-part1 branch September 7, 2017 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/plugins v3spec part1 connect #73 #381

Feature/plugins v3spec part1 connect #73 #381

DrMegavolt commented Aug 23, 2017 •

edited

Loading

codecov bot commented Aug 23, 2017 •

edited

Loading

kevinswiber Aug 29, 2017

DrMegavolt Aug 29, 2017

kevinswiber Aug 30, 2017

DrMegavolt Aug 31, 2017

kevinswiber commented Aug 30, 2017

DrMegavolt commented Aug 31, 2017

altsang Sep 1, 2017

DrMegavolt Sep 1, 2017

kevinswiber Sep 4, 2017

DrMegavolt Sep 4, 2017

DrMegavolt Sep 4, 2017

kevinswiber Sep 4, 2017

DrMegavolt Sep 4, 2017

kevinswiber Sep 4, 2017

altsang Sep 6, 2017

kevinswiber Sep 4, 2017

DrMegavolt commented Sep 6, 2017

kevinswiber commented Sep 6, 2017 •

edited

Loading

kevinswiber commented Sep 6, 2017 •

edited

Loading

altsang commented Sep 7, 2017

DrMegavolt commented Sep 7, 2017

Feature/plugins v3spec part1 connect #73 #381

Feature/plugins v3spec part1 connect #73 #381

Conversation

DrMegavolt commented Aug 23, 2017 • edited Loading

codecov bot commented Aug 23, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinswiber commented Aug 30, 2017

DrMegavolt commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrMegavolt commented Sep 6, 2017

kevinswiber commented Sep 6, 2017 • edited Loading

kevinswiber commented Sep 6, 2017 • edited Loading

altsang commented Sep 7, 2017

DrMegavolt commented Sep 7, 2017

DrMegavolt commented Aug 23, 2017 •

edited

Loading

codecov bot commented Aug 23, 2017 •

edited

Loading

kevinswiber commented Sep 6, 2017 •

edited

Loading

kevinswiber commented Sep 6, 2017 •

edited

Loading