-
Notifications
You must be signed in to change notification settings - Fork 1
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
Issue 9 standalone aietes #23
Conversation
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.
Just some minor things maybe worth discussing.
simplified standalone interface by removing some parameters adapted README
standalone/index.js
Outdated
const responseFileName = argv.json; | ||
let jsonResponse; | ||
if (responseFileName) { | ||
jsonResponse = JSON.parse(await fs.readFile(responseFileName, 'utf8')); |
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.
Should we implement something if JSON.parse throws an error because the Json inside the file is not valid?
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.
That does sound like a good idea. Though I think the only thing we can do is output a (hopefully) helpful error and exit gracefully.
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.
Here or next PR?
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.
Come to think of it: what do you mean? Just catch the error that readFile
may throw or validate the structure of the JSON read in before passing it to Aietes?
We could do both, like e.g. using this https://www.npmjs.com/package/jsonschema
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.
I'd suggest doing the validation in a separate PR though.
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.
Deal.
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.
Implemented requested changes.
Probably not yet final, but would be nice to get some feedback thoughts on how we want to implement the standalone Aietes server and what configuration options we see are "Must Have" in the initial version.