ansible-collections / community.routeros

Ansible modules for managing MikroTik RouterOS instances.
https://galaxy.ansible.com/ui/repo/published/community/routeros/
GNU General Public License v3.0
98 stars 45 forks source link

Add interface wifi paths #266

Closed cosandr closed 6 months ago

cosandr commented 7 months ago
SUMMARY

Fixes #253.

Mostly copy-pasted from interface wifiwave2 with some minor changes (added missing comment and disabled where relevant, removed openflow-switch from datapath, changed default l2mtu to 1560).

ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION

I'm unsure about the default values, but they haven't caused me any issues.

I'm unable to rename the default wifi interfaces, it fails with

Error while removing name=\"wifi1\" (ID *6): failure: not allowed to remove

I can do it from the CLI with

/interface/wifi> set wifi1 name=wifi-test

I assume the issue is having name as primary key, causing the module to try to remove it before creating a new one with the new name.

github-actions[bot] commented 7 months ago

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and the docs are now incorporated into main: https://ansible-collections.github.io/community.routeros/branch/main

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 82.99%. Comparing base (3d737d6) to head (efc11c3).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #266 +/- ## ======================================= Coverage 82.99% 82.99% ======================================= Files 32 32 Lines 4051 4051 Branches 873 873 ======================================= Hits 3362 3362 Misses 506 506 Partials 183 183 ``` | [Flag](https://app.codecov.io/gh/ansible-collections/community.routeros/pull/266/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | Coverage Δ | | |---|---|---| | [integration](https://app.codecov.io/gh/ansible-collections/community.routeros/pull/266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `66.86% <ø> (ø)` | | | [sanity](https://app.codecov.io/gh/ansible-collections/community.routeros/pull/266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `22.08% <ø> (ø)` | | | [units](https://app.codecov.io/gh/ansible-collections/community.routeros/pull/266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `82.93% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cosandr commented 7 months ago

@felixfontein I'm struggling to get this to properly remove values which I've removed from the configuration. This is starting with an empty interface wifi.

Example tasks ```yml - name: Get info community.routeros.api_info: path: interface wifi handle_disabled: omit register: __res - debug: var: __res.result - name: Configure wifi community.routeros.api_modify: path: interface wifi data: - name: wifi1 default-name: wifi1 disabled: false configuration.country: Norway - name: wifi2 default-name: wifi2 handle_absent_entries: remove handle_entries_content: remove_as_much_as_possible - name: Get info community.routeros.api_info: path: interface wifi handle_disabled: omit register: __res - debug: var: __res.result - name: Configure wifi community.routeros.api_modify: path: interface wifi data: - name: wifi1 default-name: wifi1 disabled: false - name: wifi2 default-name: wifi2 handle_absent_entries: remove handle_entries_content: remove_as_much_as_possible - name: Get info community.routeros.api_info: path: interface wifi handle_disabled: omit register: __res - debug: var: __res.result ```
Output (with diff) ``` TASK [Get info] ************************************** ok: [hapax3] TASK [debug] ***************************************** ok: [hapax3] => { "__res.result": [ { ".id": "*6", "default-name": "wifi1", "mac-address": "", "name": "wifi1", "radio-mac": "" }, { ".id": "*7", "default-name": "wifi2", "mac-address": "", "name": "wifi2", "radio-mac": "" } ] } TASK [Configure wifi] ******************************** --- before +++ after @@ -3,8 +3,9 @@ { ".id": "*6", "arp-timeout": "auto", + "configuration.country": "Norway", "default-name": "wifi1", - "disabled": true, + "disabled": false, "l2mtu": 1560, "mac-address": "", "name": "wifi1", changed: [hapax3 -> localhost] TASK [Get info] ************************************** ok: [hapax3] TASK [debug] ***************************************** ok: [hapax3] => { "__res.result": [ { ".id": "*6", "configuration.country": "Norway", "default-name": "wifi1", "disabled": false, "mac-address": "", "name": "wifi1", "radio-mac": "" }, { ".id": "*7", "default-name": "wifi2", "mac-address": "", "name": "wifi2", "radio-mac": "" } ] } TASK [Configure wifi] ******************************** ok: [hapax3 -> localhost] TASK [Get info] ************************************** ok: [hapax3] TASK [debug] ***************************************** ok: [hapax3] => { "__res.result": [ { ".id": "*6", "configuration.country": "Norway", "default-name": "wifi1", "disabled": false, "mac-address": "", "name": "wifi1", "radio-mac": "" }, { ".id": "*7", "default-name": "wifi2", "mac-address": "", "name": "wifi2", "radio-mac": "" } ] } ```

Any ideas? I don't remember running into this with other paths.

felixfontein commented 7 months ago

The module only knows how to remove values that have can_disabled=True (optionally with remove_value, depending on how to remove it via the API). Since most of the fields do not have can_disabled=True the module doesn't know how to remove them, and since you supplied handle_entries_content: remove_as_much_as_possible it does not complain in that case.

cosandr commented 7 months ago

I see, thanks! I've tried a few more things and most paths only require the primary key, everything else can be disabled, so I've removed most defaults in favor of can_disable=True. Some have to stay (interface wifi needs at least l2mtu and arp-timeout). I can't guarantee every single option works, but I think not being able to remove them at all is not expected behavior.

I'm also thinking it's better to remove them instead of duplicating rOS defaults. For example even though the docs claim arp=enabled is the default, it doesn't show up if you run export whereas it would if the module removes it and sets it back to the default value, whereas it won't show up if it's removed via can_disable=True (so it's the same as it was before the module did anything with it).

felixfontein commented 6 months ago

I don't have time to properly review this, but from a glance it looks good. If nobody objecs, I'll merge this tomorrow :)