Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Filter incoming messages on product name #21

Merged
merged 12 commits into from
Apr 18, 2024
Merged

Filter incoming messages on product name #21

merged 12 commits into from
Apr 18, 2024

Conversation

adybbroe
Copy link
Owner

@adybbroe adybbroe commented Dec 14, 2023 •
edited
Loading

This PR is introducing a requirement on the product name in the incoming messages.

  • Closes #xxxx
  • Tests added
  • Tests passed: Passes pytest
  • Passes flake8
  • Fully documented

Refactor and increase test coverage and filter incoming messages on p…
b46ba2f 
…roduct name "afimg"

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2023 •
edited
Loading

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (accc109) 82.73% compared to head (8f7ae8e) 82.71%.

Files Patch % Lines
activefires_pp/spatiotemporal_alarm_filtering.py 55.55% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
- Coverage   82.73%   82.71%   -0.03%     
==========================================
  Files          21       21              
  Lines        2554     2574      +20     
  Branches      386      389       +3     
==========================================
+ Hits         2113     2129      +16     
- Misses        410      414       +4     
  Partials       31       31
Flag Coverage Δ
unittests 82.71% <83.33%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coveralls
Copy link

coveralls commented Dec 14, 2023 •
edited
Loading

Coverage Status

coverage: 84.106% (+0.2%) from 83.947%
when pulling 8083890 on filter-incoming-messages-on-product-name
into accc109 on main.

Adam.Dybbroe added 2 commits December 15, 2023 08:46
Add option to configure which product names to accept/handle
0186211 
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Fix missing tests
8f7ae8e 
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe adybbroe self-assigned this Apr 7, 2024
@adybbroe adybbroe requested a review from mraspaud April 7, 2024 18:33
Adam.Dybbroe added 6 commits April 12, 2024 15:16
Remove unused function
0561bd5 
Signed-off-by: Adam.Dybbroe <a000680@c22526.ad.smhi.se>
hardfix to try overcome what looks like a race condition on the Githu…
8b34158 
…b CI jobs

Signed-off-by: Adam.Dybbroe <a000680@c22526.ad.smhi.se>
Try fix CI runs
ee3ec2a 
Signed-off-by: Adam.Dybbroe <a000680@c22526.ad.smhi.se>
Revert back the fake-publisher handling and put an upper limit to pos…
bd09689 
…ttroll for the CI tests to pass

Signed-off-by: Adam.Dybbroe <a000680@c22526.ad.smhi.se>
Include python 3.12 in the CI tests and skip 3.10
329e8e8 
Signed-off-by: Adam.Dybbroe <a000680@c22526.ad.smhi.se>
Adapt to latest posttroll (v1.11)
bd96a5d 
Signed-off-by: Adam.Dybbroe <a000680@c22526.ad.smhi.se>
@adybbroe adybbroe marked this pull request as ready for review April 14, 2024 11:52
@adybbroe adybbroe added the enhancement New feature or request label Apr 14, 2024
mraspaud
mraspaud reviewed Apr 17, 2024
View reviewed changes
Copy link
Collaborator

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, otherwise looks good.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
activefires_pp/spatiotemporal_alarm_filtering.py Outdated
@@ -180,6 +181,9 @@ def run(self):
elif msg.type not in ['file', 'collection', 'dataset']:
LOG.debug("Message type not supported: %s", str(msg.type))
continue
elif product_list and msg.data.get('product') not in product_list:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if there is not product in the message? is None a possible value in the product_list?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! As I have implemented it yes one needs to specify a list with at leas one product (name) in it. Before it was hardcoded to only consider one, and that should be named afimg. At SMHI we now produce two kinds of GeoJSON output, one using the default WGS84 and one using SWEREF99, the later output file names we prepend with afimg_sweref99_. We only want to notify users with the WGS84 output at the moment.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point! None is theoretically a possible name for a product, and then if the message does not contain the product name any product would pass through and give rise to a notification.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I have changed so no product name in the message will check for products named "unknown" against the configured product names. You think this is better and sufficient?

activefires_pp/spatiotemporal_alarm_filtering.py Show resolved Hide resolved
activefires_pp/tests/test_messaging.py Outdated Show resolved Hide resolved
Adam.Dybbroe added 3 commits April 17, 2024 18:09
Remove unnecessary commented out tests
8c8ed37 
Signed-off-by: Adam.Dybbroe <a000680@c22526.ad.smhi.se>
Skip Python 3.9 support
ab18a5d 
Signed-off-by: Adam.Dybbroe <a000680@c22526.ad.smhi.se>
Fix so no product name in the message results in the name with value …
8083890 
…"unknown" rather than None

Signed-off-by: Adam.Dybbroe <a000680@c22526.ad.smhi.se>
@adybbroe adybbroe requested a review from mraspaud April 17, 2024 16:22
@adybbroe
Copy link
Owner Author

Better now @mraspaud ?

mraspaud
mraspaud approved these changes Apr 18, 2024
View reviewed changes
Copy link
Collaborator

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mraspaud mraspaud merged commit f1a0524 into main Apr 18, 2024
17 of 19 checks passed
@mraspaud mraspaud deleted the filter-incoming-messages-on-product-name branch April 18, 2024 07:20
@mraspaud mraspaud mentioned this pull request Apr 18, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mraspaud mraspaud mraspaud approved these changes

Assignees

@adybbroe adybbroe

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@adybbroe @codecov-commenter @coveralls @mraspaud