zahodi / ansible-mikrotik

Mikrotik module collection for ansible
Apache License 2.0
96 stars 33 forks source link

Ip address refactor #28

Closed amatamalas closed 6 years ago

amatamalas commented 6 years ago

PR to convert the ip address module to idempotent for issue #13. I'm not 100% sure about the implementation and testing so comments are more than welcome as always.

zahodi commented 6 years ago

Thanks @amatamalas. Everything works the only thing I would like to modify is tests.

Since we are trying to make this compatible with ec2 Cloud RouterOS we are limited to only 2 physical interfaces. That's why I'm bringing up a few extra bridge interfaces to test IP addresses on interfaces.

amatamalas commented 6 years ago

No problem @zahodi. I will work on the testing based on your previous interface/bridge schema

amatamalas commented 6 years ago

@zahodi I have corrected the testing using bridges and also a very important point with the idempotent parameter. For some reason in the original PR I used "interface" when it must be "address".

zahodi commented 6 years ago

@amatamalas I actually wanted to have a discussion on this since I prefer to keep interface as the idempotent parameter. This allows to add, remove, and edit an IP address on an interface. I'm trying to think of situations where someone would need two IP addresses on single interface.

I'm open to both options.

amatamalas commented 6 years ago

@zahodi I also had a lot of doubts about which one to use and tried both of them. My conclusion was that having address as idempotent parameter allows for a much more flexible environment. You can still add and remove IPs from an interface but you can also "edit" by performing two actions remove+add. In a full production environment I don't think interfaces have multiple IP addresses but I wanted to leave it open for other implementations like testing or lab setup environments. I think that having address as idempotent allows a very simple management of the single ip per interface scenario, but we are limited to this scenario only. My prefer option would be address but I'm also open to both.

zahodi commented 6 years ago

@amatamalas I looked over my prod environment yesterday and I have have multiple instance of same IP on the different interfaces so this implementation would break the setup that I have. I think a good compromise would be to allow a user to decide which idempotent_parameter to use, something like this:

- name: set ip address on bridge2
  mt_ip_address:
    hostname:   "{{ mt_hostname }}"
    username:   "{{ mt_user }}"
    password:   "{{ mt_pass }}"
    state:      "present"
    idempotent_parameter: "interface | address"
    settings:
      interface:  "bridge2"
      address:    "192.168.88.2/24"
      network: "192.168.88.0"
senorsmile commented 6 years ago

@zahodi I REALLY like this idea... but only where it makes sense.

amatamalas commented 6 years ago

@zahodi I like that idea too. I think it's a very good way to cover all the possible scenarios. With a good documentation on how to use it and the implications of each options for who ever use it. I will work on a modification of this PR and have it done.

zahodi commented 6 years ago

@amatamalas everything looks goods. The only thing we still want to add to the tests is "check mode" compatibility. This is an example from the bridge interface tests https://github.com/zahodi/ansible-mikrotik/blob/master/tests/integration/tasks/test-bridge.yml

  failed_when: (
      not ansible_check_mode
    ) and (
      not ( bridge_settings_1 | changed )
)