-
Notifications
You must be signed in to change notification settings - Fork 62
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
Attach firewall when creating nodebalancer #143
Conversation
I think you have to run |
I have one last comment about testing, but otherwise it looks good. The one big problem with this is that it only supports creation, which is just... insufficient. I understand we're limited by the Linode API, but given this, we should really make sure this is super alpha, not something that should be used beyond some testing. (cc @schinmai-akamai) |
Turns out it should be possible to remove firewalls from nodebalancer by using DeleteFirewallDevice (thanks, Tarun, for pointing this out). So if we're going the creation route, we should also incorporate this (if possible). |
Yup, the devices on the firewalls could be updated or deleted. I will be incorporating these changes as well! |
The linodego doesn't yet have the ability to list firewalls of a nodebalancer. The update feature can be added as soon as the ability has been added (https://jira.linode.com/browse/TPT-2535) |
|
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 last bit of comment regarding the refactored tests. Feel free to merge once you incorporate the small changes I suggested.
Fixes #142
Changes:
General:
Pull Request Guidelines: