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

Added --broadcast-whitelist option #39

Closed
wants to merge 2 commits into from
Closed

Added --broadcast-whitelist option #39

wants to merge 2 commits into from

Conversation

thusser
Copy link
Contributor

@thusser thusser commented Mar 3, 2016

I added a new --broadcast-whitelist option that in principle is just a copy of --whitelist but works on the broadcaster, i.e. only hosts from a given network can subscribe to its events. It just denies clients to connect, so another Comet session with the --remote flag set to that broadcaster will try to connect forever (although in increasing intervals). This is not ideal, but I don't see a nice way around it.

I also fixed a minor but in broker.py, where log.warning is used instead of log.warn.

@jdswinbank
Copy link
Owner

Thank you for this! My initial thought is that the code looks good; I'll dive into it further as soon as I get chance.

Can you confirm what the copyright status of this code is? Are you willing and able to assign copyright to me, or do you/your employer need to be mentioned in the project's licensing statement?

@thusser
Copy link
Contributor Author

thusser commented Mar 7, 2016

It was a minor change, so yes, you can have the copyright for it. I (or my employer) don't need to be explicitly named in the licensing statement, but of course I'd appreciate seeing my name as a contributor somewhere. It's really up to you.

@jdswinbank
Copy link
Owner

Thank you! I'm reviewing this code today. Of course, I'm delighted to list you as a contributor. Currently, the only details I have for you are the name "thusser" and the e-mail address in your git commits. If you'd like me to add a full name, URL, etc, please send it my way.

@jdswinbank
Copy link
Owner

(Actually, I see your GitHub profile now has a full name -- pretty sure it didn't last time I looked -- so I'll use that.)

@thusser
Copy link
Contributor Author

thusser commented Mar 17, 2016

Just name and email is fine. And yes, I updated my profile only a couple of days ago.

@jdswinbank
Copy link
Owner

I've already merged your bug fix to master and pushed it into a new release -- many thanks!

I like the broadcaster whitelisting feature. However, I think we can make it a bit simpler, because there's no real need to make a new factory class: we can just reuse the existing WhitelistingFactory. I'd also prefer to call it "subscriber whitelist" rather than "broadcast whitelist", because I think that's a little easier for new users to follow.

Starting from your commit, I've made those changes and added some docs and tests. The result is on PR #41. Could you please take a look and check if you're happy with these changes? Thanks!

@jdswinbank
Copy link
Owner

Merged #41. Please reopen this if that doesn't scratch your itch!

@jdswinbank jdswinbank closed this Apr 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants