-
-
Notifications
You must be signed in to change notification settings - Fork 288
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
Common scheduler #1641
Common scheduler #1641
Conversation
Codecov ReportBase: 97.29% // Head: 97.52% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1641 +/- ##
==========================================
+ Coverage 97.29% 97.52% +0.23%
==========================================
Files 646 646
Lines 9753 9734 -19
==========================================
+ Hits 9489 9493 +4
+ Misses 264 241 -23
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
c00dfe3
to
6c7c3ec
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.
Thanks for this PR, great work! 🙂
I added a few comments. I'm not sure I'm confident with this PR modifying Gladys Plus code, it's changing the scope of this review to a whole different level 😅
Gladys Plus has paying customers, so I can't change things without serious real-life testing. I'm really trying to have Gladys Plus to be as stable as possible.
I hope you understand !
b89962e
to
1521eb9
Compare
I totally understand, I rollback some un-necessary changes, but some are required to use the common scheduler. |
1521eb9
to
9c3d6f4
Compare
@atrovato Thank you 🙏 Let me know when it's ready for review again |
Hi @Pierre-Gilles, I'm ready for a new review. Please find a comparison between "ol" and "new" version of tests. |
9c3d6f4
to
3dd3b47
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.
Thanks for your fixes! It's super clear to me, and I agree with the improvements in the Gateway tests, thanks for them 🙏
I have a few feedbacks but nothing really serious :)
e2cbbdb
to
da9626a
Compare
@atrovato is this ready for a new review ? Let me know :) |
da9626a
to
b101b73
Compare
I just rebased it, yes ready for new review :) @Pierre-Gilles |
b101b73
to
58039f5
Compare
@atrovato I reviewed the code and it looks good to me. But I need to test it in real-life because it's an important change! Do you have a Docker image to try it ? |
|
@atrovato I started an instance with your image. Let's see in the coming day if all jobs are executing as expected Which behavior have you tested in real life (just to know) ? Needs to be tested :
|
58039f5
to
b974303
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.
Tested on my side in real life, thanks for this PR, everything works fine! 🙏
Job #1081: Bundle Size — 7.19MiB (0%).
Metrics (no changes)
Total size by type (no changes)
|
Pull Request check-list
To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:
npm test
on both front/server)npm run eslint
on both front/server)npm run prettier
on both front/server)[ ] If you are adding a new features/services, did you run integration comparator? (npm run compare-translations
on front)[ ] If your changes modify the API (REST or Node.js), did you modify the API documentation? (Documentation is based on comments in code)[ ] If you are adding a new features/services which needs explanation, did you modify the user documentation? See the GitHub repo and the website.[ ] Did you add fake requests data for the demo mode (front/src/config/demo.js
) so that the demo website is working without a backend? (if needed) See https://demo.gladysassistant.com.NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
Gladys core and common scheduler
node-schedule
libraryscheduleJob
in Gladys scheduler (accessible from services)Changes
Now
scheduler-jobs
are crons instead of interval.