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.

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

Add an s3stalker daemon #114

Merged
merged 54 commits into from
Mar 20, 2023
Merged

Add an s3stalker daemon #114

merged 54 commits into from
Mar 20, 2023

Conversation

adybbroe
Copy link
Contributor

Add a runner or demon to stalk for new files in an S3 bucket based on the s3stalker script

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe adybbroe self-assigned this Aug 17, 2022
@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #114 (456c73b) into main (62a1d71) will increase coverage by 0.69%.
The diff coverage is 99.64%.

@@            Coverage Diff             @@
##             main     #114      +/-   ##
==========================================
+ Coverage   90.52%   91.22%   +0.69%     
==========================================
  Files          22       27       +5     
  Lines        3577     3998     +421     
==========================================
+ Hits         3238     3647     +409     
- Misses        339      351      +12     
Flag Coverage Δ
unittests 91.22% <99.64%> (?)

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

Impacted Files Coverage Δ
pytroll_collectors/tests/test_fsspec_to_message.py 93.29% <ø> (ø)
pytroll_collectors/tests/test_s3stalker.py 99.46% <99.21%> (-0.54%) ⬇️
pytroll_collectors/s3stalker.py 100.00% <100.00%> (ø)
pytroll_collectors/s3stalker_daemon_runner.py 100.00% <100.00%> (ø)
pytroll_collectors/scisys.py 62.18% <100.00%> (ø)
...l_collectors/tests/test_s3stalker_daemon_runner.py 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Adam.Dybbroe added 2 commits August 18, 2022 10:30
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe adybbroe marked this pull request as ready for review August 18, 2022 08:32
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Copy link
Member

@pnuu pnuu left a comment

Choose a reason for hiding this comment

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

Some comments, questions and suggestions :-)

bin/s3stalker_daemon.py Outdated Show resolved Hide resolved
bin/s3stalker_daemon.py Outdated Show resolved Hide resolved
bin/s3stalker_daemon.py Outdated Show resolved Hide resolved
bin/s3stalker_daemon.py Outdated Show resolved Hide resolved
bin/s3stalker_daemon.py Outdated Show resolved Hide resolved
bin/s3stalker_daemon.py Outdated Show resolved Hide resolved
bin/s3stalker_daemon.py Outdated Show resolved Hide resolved
bin/s3stalker_daemon.py Outdated Show resolved Hide resolved
bin/s3stalker_daemon.py Outdated Show resolved Hide resolved
Adam.Dybbroe and others added 6 commits December 19, 2022 12:00
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Co-authored-by: Panu Lahtinen <pnuu+git@iki.fi>
Co-authored-by: Panu Lahtinen <pnuu+git@iki.fi>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
…llectors into add-s3stalker-daemon

# Conflicts:
#	bin/s3stalker_daemon.py

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe
Copy link
Contributor Author

Damn. Moving the self._setup_and_start_communication()from the init to the run method as suggested was a bad idea I am afraid, it gives me this error:
ValueError: signal only works in main thread of the main interpreter
So, I am moving it back into the init @pnuu

@adybbroe
Copy link
Contributor Author

Damn. Moving the self._setup_and_start_communication()from the init to the run method as suggested was a bad idea I am afraid, it gives me this error: ValueError: signal only works in main thread of the main interpreter So, I am moving it back into the init @pnuu

Oh, I see now, that you only suggested moving the publisher creation to the run method, sorry will redo...

Adam.Dybbroe added 9 commits December 20, 2022 15:51
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
# Conflicts:
#	.github/workflows/ci.yaml
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Copy link
Member

@pnuu pnuu left a comment

Choose a reason for hiding this comment

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

Some new comments.

bin/s3stalker_daemon.py Outdated Show resolved Hide resolved
bin/s3stalker_daemon.py Show resolved Hide resolved
@@ -54,6 +149,7 @@ class DatetimeHolder:

def get_last_files(path, *args, pattern=None, **kwargs):
"""Get the last files from path (s3 bucket and directory)."""
kwargs['skip_instance_cache'] = True
Copy link
Member

Choose a reason for hiding this comment

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

It just seemed a bit odd to modify the kwargs as the first thing in the function. Would it be more clear that it is set where the call to this function is so there are no side-effects when the call is made? I'm fine also with this, maybe a short explanation would be nice.

pytroll_collectors/s3stalker.py Outdated Show resolved Hide resolved
pytroll_collectors/s3stalker.py Outdated Show resolved Hide resolved
pytroll_collectors/s3stalker.py Outdated Show resolved Hide resolved
pytroll_collectors/s3stalker_daemon_runner.py Outdated Show resolved Hide resolved
pytroll_collectors/s3stalker_daemon_runner.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
doc/source/index.rst Outdated Show resolved Hide resolved
@mraspaud
Copy link
Member

mraspaud commented Mar 3, 2023

@pnuu I have addressed or answered your comments...

doc/source/index.rst Outdated Show resolved Hide resolved
bin/s3stalker_daemon.py Show resolved Hide resolved
Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
@pnuu
Copy link
Member

pnuu commented Mar 9, 2023

I went through the unresolved comments, and only my two latest ones are still open.

For some reason GitHub shows some of the comments without the ability to resolve them, so the comment section is a bit messy.

Copy link
Member

@pnuu pnuu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the updates!

Just one suggestion on the S3 credential configuration.

doc/source/index.rst Show resolved Hide resolved
Co-authored-by: Panu Lahtinen <pnuu+git@iki.fi>
@pnuu pnuu merged commit 5b40ded into pytroll:main Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants