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

Handle setting subscriber nameserver to False #159

Merged
merged 12 commits into from
Jan 31, 2023

Conversation

pnuu
Copy link
Member

@pnuu pnuu commented Sep 12, 2022

It wasn't possible to disable the subscriber nameserver to False. With this PR, the argument parsing takes care of this. Also, the argument parsing and launching of the Runner have now been moved to trollflow2.launcher so we could add tests for them.

  • Tests added
  • Tests passed
  • Passes flake8 trollflow2
  • Fully documented

@pnuu pnuu added the enhancement New feature or request label Sep 12, 2022
@pnuu pnuu self-assigned this Sep 12, 2022
@pnuu pnuu requested a review from mraspaud as a code owner September 12, 2022 07:52
@codecov
Copy link

codecov bot commented Sep 12, 2022

Codecov Report

Merging #159 (ff232fa) into main (d02496d) will increase coverage by 0.00%.
The diff coverage is 96.29%.

@@           Coverage Diff           @@
##             main     #159   +/-   ##
=======================================
  Coverage   95.36%   95.37%           
=======================================
  Files          13       13           
  Lines        2652     2700   +48     
=======================================
+ Hits         2529     2575   +46     
- Misses        123      125    +2     
Flag Coverage Δ
unittests 95.37% <96.29%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
trollflow2/launcher.py 87.93% <95.12%> (+0.71%) ⬆️
trollflow2/tests/test_launcher.py 98.35% <100.00%> (+0.04%) ⬆️

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

@pnuu pnuu requested a review from gerritholl September 14, 2022 06:14
Copy link
Member

@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.

Thanks for the refactor along the way. Some minor things and tests missing, otherwise the logic looks good

trollflow2/launcher.py Show resolved Hide resolved
trollflow2/launcher.py Outdated Show resolved Hide resolved
trollflow2/launcher.py Show resolved Hide resolved
trollflow2/launcher.py Show resolved Hide resolved
Copy link
Member

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

Thanks. I left some comments on how tests and documentation could be improved.

trollflow2/launcher.py Show resolved Hide resolved
trollflow2/launcher.py Show resolved Hide resolved
trollflow2/launcher.py Outdated Show resolved Hide resolved
@pnuu pnuu requested review from mraspaud and gerritholl December 16, 2022 12:05
gerritholl
gerritholl previously approved these changes Jan 27, 2023
Copy link
Member

@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.

Not much to say, looks pretty good to me :)

trollflow2/launcher.py Outdated Show resolved Hide resolved
trollflow2/tests/test_launcher.py Show resolved Hide resolved
Comment on lines +26 to +30
It is possible to disable Posttroll Nameserver usage for the incoming
messages by starting ``satpy_launcher.py`` with command-line arguments
``-n False -a tcp://<host>:<port>`` where the host and port point to a
message publisher. Multiple publisher addresses can be given by supplying
them with additional ``-a`` switches.
Copy link
Member

Choose a reason for hiding this comment

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

Just a question here: Is this the same interface we use in other pytroll modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

Internally, yes. All the packages using create_subscriber_from_dict_config() or create_publisher_from_dict_config() are using the same settings, explained here: https://github.com/pytroll/posttroll/blob/main/posttroll/subscriber.py#L437-L438

Copy link
Member

Choose a reason for hiding this comment

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

ok! but I was actually wondering if the external/command line interface was the same?

Copy link
Member

Choose a reason for hiding this comment

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

bump @pnuu

Copy link
Member Author

@pnuu pnuu Jan 31, 2023

Choose a reason for hiding this comment

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

For the incoming messages (subscribers) I found these:

  • geographic_gatherer.py uses -n false and -i tcp://..., where -i is "inbound connection" and means the same as -a does here
  • segment_gatherer.py reads these settings from the config file (using nameservers and addresses option names)

Internally the both use posttroll.subscriber.create_subscriber_from_dict_config(), so given the same combination the nameserver handling will be the same. The names used by Posttroll are addresses and nameserver.

The -a and -n options were present in satpy_launcher.py already before this PR.

Copy link
Member

Choose a reason for hiding this comment

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

ok, we'll keep them for now then

mraspaud
mraspaud previously approved these changes Jan 31, 2023
Copy link
Member

@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

@pnuu pnuu merged commit fb59786 into pytroll:main Jan 31, 2023
@pnuu pnuu deleted the feature-listener-nameserver-false branch January 31, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants