-
-
Notifications
You must be signed in to change notification settings - Fork 383
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
feat: replacing incron with a specific script #8234
Conversation
I would like to add some integration test. But it should be reviewable @stephanegigandet |
You have successfully added a new SonarCloud configuration ``. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab. |
Codecov Report
@@ Coverage Diff @@
## main #8234 +/- ##
==========================================
+ Coverage 47.06% 47.37% +0.30%
==========================================
Files 104 105 +1
Lines 20449 20563 +114
Branches 4650 4656 +6
==========================================
+ Hits 9624 9741 +117
+ Misses 9678 9670 -8
- Partials 1147 1152 +5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -266,7 +266,7 @@ sub ensure_expected_results_dir ($expected_results_dir, $update_expected_results | |||
return 1; | |||
} | |||
|
|||
=head2 compare_to_expected_results($object_ref, $expected_results_file, $update_expected_results) { | |||
=head2 compare_to_expected_results($object_ref, $expected_results_file, $update_expected_results, $test_ref = undef) { |
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.
$test_ref is currently not used
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.
it will be used when I merge data quality knowledge panels :-D
lib/ProductOpener/Images.pm
Outdated
{ | ||
features => [ | ||
{type => 'TEXT_DETECTION'}, | ||
{type => 'LOGO_DETECTION'}, |
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.
for cropped images for ingredients and nutrition, we just need the text detection, maybe we could add an extra parameter to send_image_to_cloud_vision() so that we can state which detection we want.
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.
Oh, I see, I though it was not intended (as code was duplicated).
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.
We pay for each type of detection, so if we just need the text, then we should remove the others (which have been done on the full uncropped image already)
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.
Looks good to me
* also add cloud vision url as a parameter
Kudos, SonarCloud Quality Gate passed! |
@@ -84,6 +84,10 @@ requires 'Log::Any::Adapter::Log4perl', '>= 0.09'; # liblog-any-adapter-log4perl | |||
requires 'Action::CircuitBreaker'; | |||
requires 'Action::Retry'; # deps: libmath-fibonacci-perl | |||
|
|||
# AnyEvent |
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.
what about the EV module?
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.
AnyEvent brings the INotify module. There is no direct integration with EV.
AnyEvent is an abstraction, EV is the event implementation layer. So we need both.
@@ -634,4 +640,74 @@ sub normalize_mail_for_comparison ($mail) { | |||
return \@lines; | |||
} | |||
|
|||
=head2 fake_http_server($port, $dump_path, $responses_ref) { |
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.
Awesome!
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 second Stèphane, Awesome!
@stephanegigandet I merged this, but it means you should change the setup in production ! (remove incron task, and add a systemd entry for this new script) |
@alexgarel I'll wait until Monday ;) |
Replaces incron with a specific script.
Fixes: