arielleleon
commented
5 months ago @helen-m-lin @jtyoung84
Tests still are not at 100% but I added the following from Helen's review.
- Readme: Looks good! I'd add a bit more in terms of service overview (EventHandler, AlertBot, JobConfigs). Are there any additional dev setup instructions?
- Models: Looks good. nit: do they need to be kept together? Or can we break apart into separate files, one for each class?
- AlertBot: Looks good.
- Main: I like how clean this module is.
- Code organization (maybe?): encapsulate main(), initiate_scheduler() and initiate_observer() into a WatchdogService class to avoid passing watch_config and scheduler. Move EventHandler into its own file.
- Run_job: Do we only want to send message for failures? Do successes get logged anywhere. Code organization (maybe?): differentiate run_job module vs run_job function. Organize functions in run_job module to be part of some JobRunnerClass with vast_config and job_config as instance variables. Not sure if this works well with the job scheduler.
- Tests: generally look good. I'll take a closer look later. I wasn't able to actually run the tests though. What do I need for setup? nit: I'd remove the comments from the yml files in /resources. If they are meant as examples/ instructions, I'd move that to readMe or another file s olely used as example.
- Styling/ linting: I see some linting issues- I'm assuming that is still in progress. Do you also plan to add github actions to run unit tests and linters on PRs?
- Logging: Do you plan on adding logging in the future?
I believe I got everything but a quick look would be appreciated :)
This is the initial release of this repository. I want to make sure it makes sense to both of you. We can coordinate a meeting if it needs more explanation. There are also parts about the run_script configuration that needs to get worked out