-
Notifications
You must be signed in to change notification settings - Fork 285
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
refactor(api-server): instantiate plugins in api-server without install them #1456
refactor(api-server): instantiate plugins in api-server without install them #1456
Conversation
packages/cactus-cmd-api-server/src/main/typescript/config/config-service.ts
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #1456 +/- ##
==========================================
+ Coverage 69.80% 69.86% +0.05%
==========================================
Files 336 336
Lines 12622 12637 +15
Branches 1512 1516 +4
==========================================
+ Hits 8811 8829 +18
+ Misses 2989 2985 -4
- Partials 822 823 +1
Continue to review full report at Codecov.
|
@elenaizaguirre In the case of this PR, I think that the contents is not substantially "new feature" but "refactoring of api-server" (not including new features). |
754113c
to
d06365f
Compare
d06365f
to
86f3511
Compare
@takeutak Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elenaizaguirre Left question above
9107cbb
to
d521a1a
Compare
packages/cactus-cmd-api-server/src/main/typescript/api-server.ts
Outdated
Show resolved
Hide resolved
@elenaizaguirre Yeah that works, sorry if I wasn't clear enough, I'm just making it up as I go. :-) |
packages/cactus-cmd-api-server/src/main/typescript/api-server.ts
Outdated
Show resolved
Hide resolved
@elenaizaguirre GitHub is bugging out somehow I'm not able to respond to your comment thread directly, but I agree it's a good idea to use an enum. @petermetz Thinking about #1210, what do you think if I change this property for another one that contains a literal indicating the action to be performed? The options of this literal could be: Instantiate |
bef4775
to
acea582
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elenaizaguirre Responding to the comment I pasted below, GitHub is not letting me respond to comment thread (super annoying bug)
The response: I would say it's better to just crash so that we don't need additional if conditions and it's guaranteed that way that if we started then all the plugins are actually operational (at least to the point that we could instantiate them).
Otherwise people would always have to wonder if their endpoints are operational or not on account of that if condition.
It might be less convenient upfront (because if you have a faulty plugin you will have to adjust your configuration) but then it's more deterministic with less possible execution paths at runtime.
@petermetz
Not right now because inside the instantiatePlugin method, if plugin was undefined, an error would have been thrown when calling onPluginInit. As the code is now, this error would be caught in the second catch and would return undefined as well.
This generates doubts to me because I put the undefined value for the purpose of not to paralyze the execution of the server in case the plugin could not be installed but there is new code that throws an exception when is not possible to call onPluginInit (although right now this error is caught).
In case of installation failure, should we throw the error or just not install the plugin?
packages/cactus-cmd-api-server/src/main/typescript/api-server.ts
Outdated
Show resolved
Hide resolved
76272a3
to
12afee9
Compare
...ages/cactus-cmd-api-server/src/test/typescript/integration/plugin-import-from-github.test.ts
Outdated
Show resolved
Hide resolved
…ll them Added parameter action to PluginImport to determine if the plugin should be installed. Enabled feature to instantiate plugins from cactus-cmd-api-server without install it. Enabled feature to install packages from github Added method create in class ConfigService to override old configuration. Method newExampleConfigConvict accepts a boolean parameter to force configuration override. Closes hyperledger-cacti#1210 Signed-off-by: Elena Izaguirre <e.izaguirre.equiza@accenture.com>
12afee9
to
d223d8b
Compare
🎉 Great news! Looks like all the dependencies have been resolved: 💡 To add or remove a dependency please update this issue/PR description. Brought to you by Dependent Issues (:robot: ). Happy coding! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petermetz
Done, I have created #1527 for that purpose, but I needed to add the dist/main folder from a real plugin (instead of a single js file) because when we instantiate a plugin, it should be able to call some plugin methods. Otherwise, server startup will fail
@elenaizaguirre Great! Thank you very much for the extra steps and also special thanks for making sure that the readmes are updated with the latest config parameter schema! LGTM
Added parameter action to PluginImport to determine if the plugin
should be installed.
Enabled feature to instantiate plugins from cactus-cmd-api-server
without install it.
Enabled feature to install packages from github
Added method create in class ConfigService to override old configuration.
Method newExampleConfigConvict accepts a boolean parameter to force
configuration override.
Closes #1210
Depends on #1527
Signed-off-by: Elena Izaguirre e.izaguirre.equiza@accenture.com