-
Notifications
You must be signed in to change notification settings - Fork 11
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
Introduce authentication for zmq communications #56
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
+ Coverage 79.90% 89.59% +9.68%
==========================================
Files 17 25 +8
Lines 2210 2278 +68
==========================================
+ Hits 1766 2041 +275
+ Misses 444 237 -207 ☔ View full report in Codecov by Sentry. |
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.
First of all, great work with the refactoring!
Could you run flake8
and see if there are manageable amount of things to fix? There were some missing new-lines in at least message.py
.
Couple additional comments inline.
|
||
ten_minutes = dt.timedelta(minutes=10) | ||
zero_seconds = dt.timedelta(seconds=0) | ||
|
||
|
||
def get_configured_address_port(): | ||
return config.get("address_publish_port", DEFAULT_ADDRESS_PUBLISH_PORT) | ||
|
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.
One new-line missing. Shouldn't flake8 catch this?
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.
Using ruff now, which can't count empty lines I think
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.
Ok, should be fixed now
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 reference, the --preview
flag needs to be added to ruff for the blank lines.
try: | ||
port = int(os.environ["NAMESERVER_PORT"]) | ||
warnings.warn("NAMESERVER_PORT is pending deprecation, please use POSTTROLL_NAMESERVER_PORT instead.", | ||
PendingDeprecationWarning) | ||
except KeyError: | ||
port = DEFAULT_NAMESERVER_PORT | ||
return config.get("nameserver_port", port) |
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.
The new POSTTROLL_NAMESERVER_PORT
isn't checked even if the warning suggests it should be 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.
... or is it coming via Donfig?
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.
yes, that's Donfig
_ = settings.pop("nameserver", None) | ||
_ = settings.pop("port", None) | ||
_ = settings.pop("services", None) | ||
_ = settings.pop("addr_listener", None), | ||
_ = settings.pop("timeout", None) | ||
|
||
return Subscriber(addresses, topics=topics, message_filter=message_filter, translate=translate) | ||
return Subscriber(**settings) |
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 think the original is cleaner as well as explicit. If there are extra items in settings
the Subscriber
creation will fail.
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.
Yes, that's the point. Until now we could pass whatever and not even know it was wrong...
This PR aims at introducing authentication in the posttroll communications.