-
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
feat: add virtual-ip module #68
base: master
Are you sure you want to change the base?
Conversation
This feature would be quite useful. I am currently just copy-pasting the module into the collection I grab via ansible-galaxy. For setting up a ipalias, for some reason I need to set both uniqid and subnet to the IP that I want. If I leave either blank I get an error. |
Is your suggestion, to make this both parameter as required? |
629c381
to
65e2291
Compare
Oh, I was just making a note of the behaviour that I witnessed. I am not
familiar enough with the module to know if both of them being set is
required, or that is a bug.
At the very least, if both must be set, that should be documented.
…-Ian Robinson
On Fri, Aug 25, 2023, 11:40 AM genofire ***@***.***> wrote:
Is your suggestion, to make this both parameter as required?
—
Reply to this email directly, view it on GitHub
<#68 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACFM6H6H6A6IUZ3GVGVTM3TXXDBORANCNFSM6AAAAAAX2DZCW4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
65e2291
to
025c79f
Compare
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.
Sorry for the delay in reviewing this. Could the review items and tests get fixed? Thanks.
025c79f
to
9b72b41
Compare
4b0465d
to
3519794
Compare
Your runner/ci-pipeline is broken .... |
Looks like the github ubuntu-latest image is broken. I've reverted to ubuntu-20.04. Please rebase on master. |
3519794
to
37a50dd
Compare
|
||
|
||
VIRTUALIP_ARGUMENT_SPEC = dict( | ||
mode=dict(required=True, choices=['proxyarp', 'carp', 'ipalias', 'other'], type='str'), |
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'm not sure this should be required - is it needed for the state = absent case?
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.
Me too. Some code is not from me - ping @coffeelover
(And I do not touch that firewall part very often)
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.
IIRC a mode is required or at least one mode should be default (i would prefer ipalias).
Also for state=absent, mode is required, because only for carp and ipalias the virtual interface has to be manually removed.
) | ||
|
||
VIRTUALIP_REQUIRED_IF = [ | ||
["mode", "carp", ["uniqid", "password", "vhid", "advbase"]], |
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 would think we would need a "state", "present"
entry.
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.
Hmm, and an absent ?
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.
It is needed for both states.
|
||
return obj | ||
|
||
def _validate_params(self): |
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.
Is there no validation to be 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.
We do not have some, should we delete that function (replace with pass
) and hope that somebody else write some?
Or is it an blocker?
Hi, |
I'm not aware of there ever being a pfsense_virtualip moduile in ansible-pfsense or pfsensible/core. But yeah, this should be called |
37a50dd
to
5ad8059
Compare
solve #47