-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add default gateway functionnality #99
Conversation
Hi. Here is my test playbook
I hope it helps. Regards; Nicolas |
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.
Thanks for the submission. Can you address the test failures and other review comments. Thanks.
Sorry for the mistakes.. I don't PR often.. |
No need to apologize, everyone has to start somewhere. Thank you for working on this. |
choices: [ "present", "absent", "read" ] | ||
default: read |
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.
So, read
is new, and not something I've seen used in ansible modules before. Can you point to any similar usage?
I'm also leaning more towards simply dropping state
altogether and just using the None
option to express that.
Finally, we generally go with lowercase options so lets use automatic
and none
.
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 wanted that too.. but what to do if you just want the current default gateway ?
Also I have to rework my code. I just realized there was an issue with dynamic gateways..
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.
But that's just not really how ansible works. You declare the state that you want. If you don't want to change anything, don't use this task.
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.
So if I get it right..
- No more "state"
- just "name" with "none" for absent
- no read
- lowercase
I could implement the fact that it accepts no option at all and return both v4 and v6 default gateway..
I implemented a find_default_gateway method because I needed it.. I feel it would be better placed in the pfsense class .. thus I did it.
Ok done !
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 checks also found some trailing whitespace to clean up. Otherwise I think we are close. Thanks for working on 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.
Looks good. I have a couple fixes to add and then I'll merge. Thanks.
Hello. Thanks. |
Change "inet4" to "inet" and remove the unused debug return. |
No description provided.