Update CLI implementation with modern tooling and conventions #1456
Conversation
cli.ts log now enables by default error and warning messages.
As a result the responsibility of managing the initial configuration for logging has been moved outside the DefaultServient.
WARNING: Drops support for running remote scripts from the Default Servient Thing. This feature was rarely used and controversial due to security implications. Users wanting to run remote scripts can easily implement this feature in their own Servient by extending the CLI Servient.
now envs are treated as config parameters
relu91
requested review from
danielpeintner,
egekorkan and
sebastiankb
as code owners
|
There is a small problem in the CLI tests due to some file shared across different unit tests. I'll fix them asap. They should not require changes to the business logic of the new CLI. |
| private async startBroker() { | ||
| return new Promise<void>((resolve, reject) => { | ||
| if (this.brokerURI == null) { | ||
| throw new Error("Unexpected configuration state broker was started even if brokerURI is null"); |
There was a problem hiding this comment.
| throw new Error("Unexpected configuration state broker was started even if brokerURI is null"); | |
| throw new Error("Unexpected configuration state. Broker was started but brokerURI is null"); |
| if (err instanceof InvalidArgumentError) { | ||
| throw err; | ||
| } | ||
| throw new InvalidArgumentError(`\nError reading config file: ${err}`); |
There was a problem hiding this comment.
| throw new InvalidArgumentError(`\nError reading config file: ${err}`); | |
| throw new InvalidArgumentError(`Error reading config file: ${err}`); |
I don't think we should start with a new line... any reason for that?
| export function parseIp(value: string, previous: string) { | ||
| if (!/^([a-z]*|[\d.]*)(:[0-9]{2,5})?$/.test(value)) { | ||
| throw new InvalidArgumentError("Invalid host:port combo"); | ||
| } | ||
|
|
||
| return value; | ||
| } |
There was a problem hiding this comment.
Several comments:
- Parameter previous is never used
- Does it work with ipv6
- do we need the code at all? To me, if a wrong IP is given it should/will fail somewhere else and it is not our job to test valid IPs..
danielpeintner
left a comment
danielpeintner
left a comment
There was a problem hiding this comment.
Note: I did not fully check nor run it locally, but I would like to make some comments:
- THANK YOU for the effort. I think it is very helpful to get rid of VM2
- I am surprised that no file has been removed (can it be?)
- some more comments, see below
| @@ -0,0 +1,2 @@ | |||
| src/generated | |||
| test/resources | |||
There was a problem hiding this comment.
Shouldn't we use the global .gitignore?
Moreover, we add test/resources which in turn says .gitkeep with no files? Is the empty folder needed!?
| }, | ||
| "scripts": { | ||
| "build": "tsc -b", | ||
| "build:json": "node import-json.js", |
There was a problem hiding this comment.
May I suggest renaming the file to something like
import-json-configuration.js ... or even leave json out.
Initially, I wasn't sure what kind of json gets imported!?
"build:json" --> I don't think the name conveys what is done...
2 participants
I know that this PR will be long to review, but it has been sitting on my pc for a while, and it is time to push the changes. The primary goal was to get rid of the VM2 module, which is now deprecated and audited as a critical security risk. I used the opportunity to completely revise the implementation and make it:
In summary, this PR delivers a significant refactor and several quality-of-life improvements to the cli module, primarily focusing on simplifying configuration, execution, and improving code organization.
Key Changes and Improvements
WOT_SERVIENT_SERVIENT_CLIENTONLY->servient.clientOnly)Open Point for Discussion
Given the extensive changes and the focus on the primary user experience:
wot-servient, is potentially surprising for regular users as it differs from the package name. I propose renaming it tonode-wotto align with the project name and make it feel less surprising.