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

Support binding IPv4 AND IPv6 instead of just either one or the other #2593

Merged
merged 4 commits into from
Sep 3, 2024

Conversation

Omar007
Copy link
Contributor

@Omar007 Omar007 commented Jul 28, 2024

At the moment the configuration is limiting input to either an IPv4 address or an IPv6 address even though the underlying waitress server has no problem binding both at the same time.
This updates the configuration to pass the wildcard value on to the waitress server so Bazarr can serve in both single and dual stack environments ootb.

@morpheus65535
Copy link
Owner

Thanks for your contribution and sorry for the delay.

@morpheus65535
Copy link
Owner

@Omar007 can you take a look at my review and comments?

@Omar007
Copy link
Contributor Author

Omar007 commented Aug 28, 2024

@morpheus65535 which ones where? I don't see any 🤔

bazarr/app/logger.py Show resolved Hide resolved
bazarr/app/server.py Show resolved Hide resolved
@morpheus65535
Copy link
Owner

@morpheus65535 which ones where? I don't see any 🤔

Sorry I forgot to submit them 🤦‍♂️

@morpheus65535 morpheus65535 merged commit 4e365c6 into morpheus65535:development Sep 3, 2024
2 checks passed
@morpheus65535
Copy link
Owner

Thanks!

@JaiZed
Copy link
Collaborator

JaiZed commented Sep 16, 2024

There is a typo in logger.py at line 65:
if record.level != loggin.ERROR:

The name logging is misspelled as loggin without the letter g.

Was this part of the code tested?
How did this pass through the automated checks?!

Also, what happens if we get a CRITICAL log message?
It looks like it will be filtered out if the line above is corrected.
Is that desired?

@Omar007
Copy link
Contributor Author

Omar007 commented Sep 16, 2024

Was this part of the code tested?

It was locally but looks like this somehow slipped between the commits/push. I have it correct locally so I think I just didn't commit it properly, that's my bad 🙇

How did this pass through the automated checks?!

This I have no idea for. The initial mistake is def. on me but I do feel like it should've been caught by something; as far as I can tell there is a workflow present that runs the application 🤔

Also, what happens if we get a CRITICAL log message?
It looks like it will be filtered out if the line above is corrected.
Is that desired?

I'd say that is not desired. I've pushed a new MR to deal with the logging fallout of this one; #2663. Please take a look at that.

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