Feature/initial #1
Conversation
| "nhsuk-bunyan-logger": "^1.4.0", | ||
| "node-schedule": "^1.2.4", | ||
| "pretty-hrtime": "^1.0.3", | ||
| "request-promise-native": "^1.0.5", |
There was a problem hiding this comment.
there's a warning on install of warning "[email protected]" has unmet peer dependency "request@^2.34". I think you also need to explicitly add request too.
There was a problem hiding this comment.
Yeah I think that is more a problem with the configuration of the package in question. https://github.com/request/request-promise-native/blob/master/package.json#L41 specifies [email protected] is required but I'd hazard a guess that it should be more lenient than that and probably be ^2.34.0 or something similar to signify any version 2.
More background on peer dependencies.
Given request is installed, the package works and there are plenty of issues about yarn not installing peer dependencies I'd be inclined to not do anything. Unless that isn't acceptable?
| @@ -0,0 +1,29 @@ | |||
| function getMergedDataFilename() { | |||
| const prefix = process.env.NODE_ENV === 'production' ? '' : 'dev-'; | |||
There was a problem hiding this comment.
The environment check was removed in the sexual-health-information-etl by allowing the filename to be specified as an env var, and having it set with a dev- prefix in the local compose file. Might be worth doing the same here.
README.md
Outdated
| ## Running the application and scheduling | ||
|
|
||
| The application will run at startup and then on a daily basis, while the | ||
| container continues to run. . The time of day defaults to 7:15am, and can be |
app.js
Outdated
|
|
||
| log.info(`Scheduling sexual health service data merge with rule '${scheduleConfig.getSchedule()}'`); | ||
| schedule.scheduleJob(scheduleConfig.getSchedule(), async () => { | ||
| await mergeAndUpload(); |
There was a problem hiding this comment.
Exceptions thrown from the await are not handled and will result in an unhandled promise rejection, terminating the app.
| data: { | ||
| dir: { | ||
| current: './data/current', | ||
| latest: './data/latest', |
There was a problem hiding this comment.
The current and latest folders do not exist causing an exception to be thrown during download.
There was a problem hiding this comment.
Mmh, I thought I'd tested that. I'll get it sorted.
| @@ -0,0 +1,19 @@ | |||
| const azureService = require('./azureService'); | |||
There was a problem hiding this comment.
Once the azure-data-service package is available this and the the azureService could be replaced with the AzureDataService class
There was a problem hiding this comment.
Yeah, I was thinking the same.
st3v3nhunt
force-pushed
the
feature/initial
branch
from
4a4b472 to
4379a23
Compare
No description provided.