-
Notifications
You must be signed in to change notification settings - Fork 553
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
Suggestion: Explicit disallow option for .allow_net_connect! #1058
Comments
Oof, I just realized you have a specific mailing list for suggestions, I will close this issue and ask there. My apologies |
Actually, the mailing list seems pretty dead, and maybe broken (I started a conversation, but do not see it). Is this the best place to propose suggestions? If the maintainers think this is a good idea, I may try to take this on myself, but I wanted to be sure it would be helpful before taking it on. |
@sandfortw Thank you for suggesting that feature. I agree it makes sense to add it. I'm not sure about the However, for consistency with the existing API, I'm inclined to stick with If you're interested in submitting a pull request for this feature, I would greatly appreciate it. |
@bblimke Excellent, I will get on that. |
Currently, in the code,
.disable_net_connect
allows users to specify explicitly allowed connections. However, the opposite is not true. If I wanted to.allow_net_connect!
on everything, but disallow specifics ones, there is currently no way to do this using the .allow_net_connect! method, which seems unintuitive to me .The specific example use case that I'm thinking of, would be using Webmock in a development environment, where you want to allow all API calls by default, but perhaps disable some specific costly/annoying ones. I don't know which APIs I might add in the future, so I don't want to have to disallow all by default, because then I will have to configure Webmock in development every time.
What do you think? I'm a pretty new developer, so there's a strong possibility that I'm missing something, or that this feature is already covered somehow, so please let me know if this is a wrongheaded idea.
to better match:
The text was updated successfully, but these errors were encountered: