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

Plugins/Protocols manifest.json overhaul #1181

Merged
merged 13 commits into from Sep 4, 2018

Conversation

Projects
None yet
6 participants
@scottinet
Member

scottinet commented Aug 10, 2018

What does this PR do?

Re-architecture the way plugins manifest.json files are handled:

  • The package.json file was required for plugins (but not for protocols), but Kuzzle actually doesn't care if a plugin is a valid NPM module or not
  • Kuzzle now displays (lengthy) warning messages during startup to encourage plugin developers to expose a manifest file: manifests will become required in the next major release of Kuzzle
  • Plugins package.json files and Protocols protocol property are now deprecated fallbacks to manifests files, and only used by Kuzzle if necessary
  • Protocols now also need to expose a manifest file
  • Manifests files are now thoroughly checked to make sure that their format is correct

List of currently supported manifest files properties:

  • name (required): Plugin/Protocol identifier. Names can only contain lowercase letters, numbers, hyphens and underscores. If the name is empty or absent, Kuzzle falls back to the package.json file (plugins) or the protocol property (protocols)
  • kuzzleVersion: must be a non-empty string in semver format, limiting the range of Kuzzle versions supported by this Plugin/Protocol. If not set, a warning is displayed on the console, and Kuzzle assumes that the Plugin/Protocol is compatible only with Kuzzle v1.x
  • privileged: must be a boolean (no change made)

(internal backlog task: KZL-430)

Other changes

  • Unit tests improvements:
    • Mocha's reporter has been set to dot instead of spec. This makes easier to spot uncaught exceptions or rejected promises, and thus makes testing safer
    • All console prints during unit-tests have been mocked to keep the tests results clean and easy to follow

Boyscout

  • Remove a call to fs.existsSync, which is deprecated
  • Kuzzle's version in the kuzzle.config object is now handled by the configuration modules, instead of being injected by Kuzzle: better separation of concerns + makes some unit tests easier by allowing the KuzzleMock object to expose the current Kuzzle version at no cost
  • Fix a promise that was rejected with a non-error object in the auth controller unit tests
@@ -699,7 +693,7 @@ class PluginsManager {
* @throws {PluginImplementationError} If strategies registration fails
*/
_initAuthenticators (plugin) {
const errorPrefix = `[${plugin.name}]:`;
const errorPrefix = `[${plugin.manifest.name}]:`;
// @todo the _.isNil test need to be removed as soon as the

This comment has been minimized.

@kuzzle

kuzzle Aug 10, 2018

INFO Complete the task associated to this TODO comment. rule

@kuzzle

kuzzle Aug 10, 2018

INFO Complete the task associated to this TODO comment. rule

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 10, 2018

Codecov Report

Merging #1181 into 1-dev will increase coverage by 0.12%.
The diff coverage is 97.27%.

Impacted file tree graph

@@            Coverage Diff            @@
##           1-dev    #1181      +/-   ##
=========================================
+ Coverage   93.8%   93.92%   +0.12%     
=========================================
  Files         91       94       +3     
  Lines       6484     6538      +54     
=========================================
+ Hits        6082     6141      +59     
+ Misses       402      397       -5
Impacted Files Coverage Δ
lib/api/kuzzle.js 82.85% <ø> (-0.33%) ⬇️
lib/api/core/plugins/manifest.js 100% <100%> (ø)
lib/api/core/entrypoints/embedded/manifest.js 100% <100%> (ø)
lib/config/index.js 89.47% <100%> (+0.58%) ⬆️
lib/api/core/entrypoints/embedded/index.js 97.68% <100%> (-0.02%) ⬇️
lib/api/core/abstractManifest.js 100% <100%> (ø)
lib/api/core/plugins/pluginsManager.js 87.05% <93.75%> (-0.18%) ⬇️
... and 1 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 32d8793...eb404a5. Read the comment docs.

codecov-io commented Aug 10, 2018

Codecov Report

Merging #1181 into 1-dev will increase coverage by 0.12%.
The diff coverage is 97.27%.

Impacted file tree graph

@@            Coverage Diff            @@
##           1-dev    #1181      +/-   ##
=========================================
+ Coverage   93.8%   93.92%   +0.12%     
=========================================
  Files         91       94       +3     
  Lines       6484     6538      +54     
=========================================
+ Hits        6082     6141      +59     
+ Misses       402      397       -5
Impacted Files Coverage Δ
lib/api/kuzzle.js 82.85% <ø> (-0.33%) ⬇️
lib/api/core/plugins/manifest.js 100% <100%> (ø)
lib/api/core/entrypoints/embedded/manifest.js 100% <100%> (ø)
lib/config/index.js 89.47% <100%> (+0.58%) ⬆️
lib/api/core/entrypoints/embedded/index.js 97.68% <100%> (-0.02%) ⬇️
lib/api/core/abstractManifest.js 100% <100%> (ø)
lib/api/core/plugins/pluginsManager.js 87.05% <93.75%> (-0.18%) ⬇️
... and 1 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 32d8793...eb404a5. Read the comment docs.

x_x
@kuzzle

This comment has been minimized.

Show comment
Hide comment
@kuzzle

kuzzle Aug 10, 2018

SonarQube analysis reported 1 issue

  • INFO 1 info

Watch the comments in this conversation to review them.

kuzzle commented Aug 10, 2018

SonarQube analysis reported 1 issue

  • INFO 1 info

Watch the comments in this conversation to review them.

@benoitvidis

That looks like quite a lot of code to me for quite a simple feature.

Moreover, I am a little concerned that plugins and protocols share some features, as they are fundamentally different.
As an example, this is probably already the case, but imo, protocols init should not throw a PluginImplementationError in case of failure.

Show outdated Hide outdated lib/api/core/plugins/manifest.js Outdated
Show outdated Hide outdated lib/api/core/entrypoints/embedded/index.js Outdated
Show outdated Hide outdated lib/api/core/plugins/manifest.js Outdated
Show outdated Hide outdated lib/api/core/plugins/manifest.js Outdated
@scottinet

This comment has been minimized.

Show comment
Hide comment
@scottinet

scottinet Aug 21, 2018

Member

About the quantity of code: this is mainly due because we already handled manifest files, but:

  • this was hard-coded in the (very) lengthy loadPlugins function
  • manifests were used as a fallback to package.json, and this PR reverses that order
  • it was only made for plugins, not for protocols

Also, I found out that manifest values weren't properly tested. All in all, I agree: a lot of code for a simple feature, but that's mainly existing code that was moved around or rearranged, and a lot of new errors are added to make plugin development easier to understand :-)

Member

scottinet commented Aug 21, 2018

About the quantity of code: this is mainly due because we already handled manifest files, but:

  • this was hard-coded in the (very) lengthy loadPlugins function
  • manifests were used as a fallback to package.json, and this PR reverses that order
  • it was only made for plugins, not for protocols

Also, I found out that manifest values weren't properly tested. All in all, I agree: a lot of code for a simple feature, but that's mainly existing code that was moved around or rearranged, and a lot of new errors are added to make plugin development easier to understand :-)

@scottinet

This comment has been minimized.

Show comment
Hide comment
@scottinet

scottinet Aug 21, 2018

Member

@benoitvidis > I implemented all your requested changes, except for the "privileged mode as a default for protocols" remark. After thinking about it, I think we are (somewhat) agreeing that this mode has no sense in itself for protocols, since they have access to the kuzzle object anyway. So the following question remains: should I expose a kuzzle object on purpose or not (I think we shouldn't, and that this "mode" has to go)

Member

scottinet commented Aug 21, 2018

@benoitvidis > I implemented all your requested changes, except for the "privileged mode as a default for protocols" remark. After thinking about it, I think we are (somewhat) agreeing that this mode has no sense in itself for protocols, since they have access to the kuzzle object anyway. So the following question remains: should I expose a kuzzle object on purpose or not (I think we shouldn't, and that this "mode" has to go)

@scottinet

This comment has been minimized.

Show comment
Hide comment
@scottinet

scottinet Aug 22, 2018

Member

After sleeping on it, it occured to me that the privileged mode can only be activated by plugins anyway, the property does nothing to protocols. So I moved that part to the plugin-specific manifest class, the property is now completely ignored by protocols.
This should put our argument to rest.

Member

scottinet commented Aug 22, 2018

After sleeping on it, it occured to me that the privileged mode can only be activated by plugins anyway, the property does nothing to protocols. So I moved that part to the plugin-specific manifest class, the property is now completely ignored by protocols.
This should put our argument to rest.

@Aschen Aschen changed the base branch from 1.x to 1-dev Sep 4, 2018

@Aschen

Aschen approved these changes Sep 4, 2018

@scottinet scottinet merged commit 5b8b781 into 1-dev Sep 4, 2018

4 of 5 checks passed

LGTM analysis: JavaScript Analysis Failed (could not build the merge commit)
Details
codecov/project 93.92% (+0.12%) compared to 32d8793
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
sonarqube SonarQube reported 1 issue, no criticals or blockers

@scottinet scottinet deleted the KZL-130/plugin-manifest-overhaul branch Sep 4, 2018

This was referenced Sep 12, 2018

scottinet added a commit that referenced this pull request Sep 13, 2018

Release 1.5.0 (#1194)
* fixes #1125 - aggs & sort on user search (#1144)

* Add AdminController (#1129)

* KZL-143 KZL-145 add admin controller and reset cache action

* Boyscout: fix crash because kuzzle try to load protocols in simplefiles

* Add tests for adminController

* Add resetKuzzleData and resetSecurity

* fix features for admin controller

* Add resetDatabase

* fix tests

* fix tests

* fix tests

* Add default rights restrictions for admin controller

* Fix test

* Fix test

* Use API for all actions except dump

* Add generateDump

* Fix tests and remove old cli tests

* Remove reset security scenario

* Make sonarqube happy

* Remove references to cliController

* Fix features

* fix internal error

* remove internal broker

* re-add shutdown

* fix test

* Put shutdown in adminController

* Reset defaults roles and profiles with resetSecurity

* replace ws by uws

* Nitpicking

* increase coverage

* increase coverage

* increase coverage

* fix linter

* increase coverage

* put back proxyBroker

* fix tests

* remove tests

* fix tests

* nitpicking

* Updates for PR

* Put back defaut config

* Disable dump if it is specified in config

* fix message in cli

* add truncate() method on repositories + refactor admin controller to use truncate()

* fix admin controller

* fix test

* Add Janitor core component

* return number of deleted objects in truncate

* fix linter

* catch TypeError for scanStream

* fix linter

* Please sonarqube

* fix linter

* Nitpicking

* Updates for PR

* update headers

* fix linter

* fix tests

* deprecate "constructor" auth option and replace it with "authenticator" (#1145)

* deprecate "constructor" auth option and replace it with "authenticator"

* fix sonarqube issues

* Add action and controller in request input for http events (#1143)

* Add action and controller in request input for http events

* put back some code

* nitpicking

* Add forgotten files --'

* Test Kuzzle against different node LTS versions (#1147)

* set ulimits for elasticsearch (#1148)

* Update kuzzle-common-objects module (#1150)

* adapt tests to new common objects version

* dependencies update

* npm audit fix

* update to plugin auth local 5.1.3 (#1149)

* update to plugin auth local 5.1.3

* adapt tests to new common objects version

* dependencies update

* npm audit fix

* prevent a breaking change when upgrade from a pre-1.4.0 version

* Revert "prevent a breaking change when upgrade from a pre-1.4.0 version"

This reverts commit 8328f50.

* prevent a breaking change occuring when upgrading Kuzzle with old config files (#1154)

* Inject protocol informations into the RequestContext object (#1155)

* inject protocol info into requestcontext

* fix incoming context infos coming from the proxy

* revert debug modification

* no need to manually copy incoming headers

* update unit tests

* fix sonarqube issu

* jsdoc fix

* fixes #1146, passport login request missing headers (#1153)

* fixes #1146, passport login request missing headers

* oauth - add some comments on arbritraty method & url

* Add script to build and push docker images (#1152)

* Add script to build and push docker images

* Add script to build and push docker images

* Fix build script

* add comment to secret env

* Use kuzzleteam account for dockerhub

* Update subnmodule

* Dont build and push on hotfix merge

* fix #1161 (#1162)

* fix startup message (#1163)

* Fix #1131: listCollections limited to the provided collection name, when specified (#1157)

* fixes #1156 - missing version in swagger output (#1164)

* add token infos to login response (#1171)

* disable dump by default (#1172)

* disable dump by default

* fix tests:

* fix #447: allow to search document with empty query through a HTTP GET request

* fix unit test

* Replace ParseError errors with BadRequestError objects (#1158)

* replace ParseError (deprecated) with BadRequestError

* fix #1168: remove useless CLI --port option  (#1177)

* fix #1168

* remove useless --port option in CLI start command

* fix #1174 (#1176)

* Fix #1115: Standardize the scrollId return value (#1173)

* Standardize the scrollId return value

* typo

* typo

* Initialize repositories before plugins (#1167)

* Initialize repositories before plugins

* remove one then() level

* Use local multi-stage build for kuzzleio/plugin-dev and kuzzleio/kuzzle (#1178)

* Update Dockerfile and build-docker-images script\n Now with multi-stage build and support for 1.x and 2.x development branch
* Remove pm2 from prod image
* Use latest node 8 version

* Add metadata to published documents (#1186)

* KZL 454 - Allow subscribe filters on metadata (#1185)

* Do not cache requests with publish(), call koncorde two times instead

* Add functionnal test for subscription filter on metadata

* Add LGTM.com code quality badges (#1187)

* KZL 290 - Add fixtures for securities (#1182)

* Move mappings and fixtures loading to Janitor

* Fix bug in repository#delete when object is not present

* Add ability to load roles, profiles and users to Janitor and CLI

* Add test for Janitor#loadSecurities

* remove .only from test

* Fix #1184: Better error message when object not found in repositories
Refactor repository loadOneFromDatabase method to reject with an explicit 404 when not found
Allow the old api usage which return a null object when not found for SecurityController#persistUser and PluginRepository#load

* Fix test

* rename getUserId

* Update for PR

* refactor janitor _create* method

* Please sonarqube

* typo

* Plugins/Protocols manifest.json overhaul (#1181)

* wip

* change reporter to dot

* fix polluting console outputs

* upgrade unit tests to make them handle the new plugin structure

* new manifest.json handling tests+fixes

* fix sonarqube issue

* x_x

* implement @benoitvidis asked changes

* fix circular json in manifest objects

* add control to manifest stringification

* remove privileged mode for protocols

* Update docker build script for new branch name 1-dev (#1190)

* Update images build script

* push only if we are on the major build

* remove test

* update default plugins (#1193)

* KZL-174 Expose to plugins a SDK instance directly mapped to the funnel (#1179)

* wip

* KZL-177: custom "funnel" network wrapper for SDK

* KZL-178: Export javascript SDK to plugins

* refactor executePluginRequest: now returns a promise

* KZL-177: revert funnelcallController => now sdk wrapper calls directly funnel.executePluginRequest

* mark `pluginContext.accessor.execute` as deprecated

* clean code

* update package-lock

* fix comment

* tix typo

* dependencies update

* remove debug config

* Release 1.5.0

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