API update #128

therfert · 2022-11-13T12:18:45Z

SUMMARY

Suport for API fields that can be disabled and have a default at the same time.
Support for API path: interface gre

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

api_modify
api_info

ADDITIONAL INFORMATION

The code makes sure the default value is used in case the field isn't specified in the data and handle_entries_content isn't ignore. It also covers a situation when the field is currently disabled in ROS.
It will disable the field only when it's explicitly defined (as empty value or key with exclamation mark prefix).
See interface gre field keepalive as an example where this is required.

…alue at the same time Signed-off-by: Tomas Herfert <herfik>

Signed-off-by: Tomas Herfert <herfik>

codecov · 2022-11-13T12:21:16Z

Codecov Report

Merging #128 (41bbf44) into main (29247fa) will decrease coverage by 0.22%.
The diff coverage is 64.70%.

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
- Coverage   87.56%   87.34%   -0.23%     
==========================================
  Files          28       28              
  Lines        3499     3516      +17     
  Branches      619      626       +7     
==========================================
+ Hits         3064     3071       +7     
- Misses        308      313       +5     
- Partials      127      132       +5

Flag	Coverage Δ
integration	`66.86% <ø> (ø)`
sanity	`22.29% <0.00%> (-0.16%)`	⬇️
units	`87.38% <64.70%> (-0.24%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
plugins/modules/api_info.py	`83.09% <ø> (-6.14%)`	⬇️
plugins/modules/api_modify.py	`79.09% <33.33%> (-0.82%)`	⬇️
plugins/module_utils/_api_data.py	`95.91% <100.00%> (ø)`
tests/unit/plugins/module_utils/test__api_data.py	`100.00% <100.00%> (ø)`
tests/unit/plugins/modules/fake_api.py	`81.28% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2022-11-13T12:22:33Z

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

therfert · 2022-11-13T12:36:21Z

Hi @felixfontein! Could you please re-run the checks? (the sanity failed because of some network issues again). Thanks

felixfontein

I don't see how default and can_disable can be used together. If there is a default and the value is not returned by the API, the modules treat the value of that field as the default value. If the field can be disabled and is not returned by the API, it is treated as disabled. If a field both has a default and is specified as 'can_disable', how should the module now handle the case that the value is not returned by the API?

plugins/module_utils/_api_data.py

therfert · 2022-11-13T17:01:47Z

I don't see how default and can_disable can be used together. If there is a default and the value is not returned by the API, the modules treat the value of that field as the default value. If the field can be disabled and is not returned by the API, it is treated as disabled. If a field both has a default and is specified as 'can_disable', how should the module now handle the case that the value is not returned by the API?

See the keepalive field of an item in interface gre.
When the item is created (and the value of keepalive isn't specified), it is created with the default value. The API returns the filed with the default value.

However the field can be explicitly disabled and then field isn't returned by the API.

See the following:

[root@new-konesin] /interface/gre[add]> add remote-address=1.2.3.4
[root@new-konesin] /interface/gre> export verbose 
<snip>
/interface gre
add allow-fast-path=yes clamp-tcp-mss=yes disabled=no dont-fragment=no dscp=inherit keepalive=10s,10 local-address=0.0.0.0 mtu=auto name=gre-tunnel7 remote-address=1.2.3.4

$ curl -s -k -u root "https://192.168.64.6/rest/interface/gre" | jq
Enter host password for user 'root':
[
  {
    ".id": "*2B",
    "actual-mtu": "1476",
    "allow-fast-path": "true",
    "clamp-tcp-mss": "true",
    "disabled": "false",
    "dont-fragment": "no",
    "dscp": "inherit",
    "keepalive": "10s,10",
    "local-address": "0.0.0.0",
    "mtu": "auto",
    "name": "gre-tunnel7",
    "remote-address": "1.2.3.4",
    "running": "false"
  }
]

[root@new-konesin] /interface/gre> unset 0 keepalive 
[root@new-konesin] /interface/gre> export verbose 
<snip>
/interface gre
add allow-fast-path=yes clamp-tcp-mss=yes disabled=no dont-fragment=no dscp=inherit !keepalive local-address=0.0.0.0 mtu=auto name=gre-tunnel7 remote-address=1.2.3.4

$ curl -s -k -u root "https://192.168.64.6/rest/interface/gre" | jq
Enter host password for user 'root':
[
  {
    ".id": "*2B",
    "actual-mtu": "1476",
    "allow-fast-path": "true",
    "clamp-tcp-mss": "true",
    "disabled": "false",
    "dont-fragment": "no",
    "dscp": "inherit",
    "local-address": "0.0.0.0",
    "mtu": "auto",
    "name": "gre-tunnel7",
    "remote-address": "1.2.3.4",
    "running": "true"
  }
]

Here you can see how api_modify works (with the proposed change):

Module parameters:

    path: interface gre
    handle_absent_entries: remove
    handle_entries_content: remove
    data:
      - name: test
        remote-address: 1.2.3.4

First run

TASK [mikrotik : Mikrotik api_modify] *******************************************************************************************************************************************************
Neděle 13 listopadu 2022  20:04:32 +0100 (0:00:01.345)       0:00:01.391 ****** 
--- before
+++ after
@@ -1,3 +1,17 @@
 {
-    "data": []
+    "data": [
+        {
+            ".id": "*2D",
+            "allow-fast-path": true,
+            "clamp-tcp-mss": true,
+            "disabled": false,
+            "dont-fragment": false,
+            "dscp": "inherit",
+            "keepalive": "10s,10",
+            "local-address": "0.0.0.0",
+            "mtu": "auto",
+            "name": "test",
+            "remote-address": "1.2.3.4"
+        }
+    ]
 }

changed: [new-konesin] => (item=interface gre)

Second run (module is idempotent):

TASK [mikrotik : Mikrotik api_modify] *******************************************************************************************************************************************************
Neděle 13 listopadu 2022  20:04:48 +0100 (0:00:01.518)       0:00:01.566 ****** 
ok: [new-konesin] => (item=interface gre)

disabling keepalive - new parameters:

    path: interface gre
    handle_absent_entries: remove
    handle_entries_content: remove
    data:
      - name: test
        remote-address: 1.2.3.4
        keepalive:

First run

TASK [mikrotik : Mikrotik api_modify] *******************************************************************************************************************************************************
Neděle 13 listopadu 2022  20:14:12 +0100 (0:00:01.670)       0:00:01.723 ****** 
--- before
+++ after
@@ -7,7 +7,6 @@
             "disabled": false,
             "dont-fragment": false,
             "dscp": "inherit",
-            "keepalive": "10s,10",
             "local-address": "0.0.0.0",
             "mtu": "auto",
             "name": "test",

changed: [new-konesin] => (item=interface gre)

second run (module is idempotent)

TASK [mikrotik : Mikrotik api_modify] *******************************************************************************************************************************************************
Neděle 13 listopadu 2022  20:14:18 +0100 (0:00:01.371)       0:00:01.415 ****** 
ok: [new-konesin] => (item=interface gre)

if I remove the "keepalive:" from the data now, it would again set the default as on the first run.

current behaviour (without the proposed change)

1st option: `'keepalive': KeyInfo(default='10s,10')`

Module is idempotent and default is used correctly, however there is no way to disable the field.
Task returns an error:

TASK [mikrotik : Mikrotik api_modify] *******************************************************************************************************************************************************
Neděle 13 listopadu 2022  20:08:01 +0100 (0:00:01.364)       0:00:01.407 ****** 
failed: [new-konesin] (item=interface gre) => {"ansible_loop_var": "item", "changed": false, "item": {"data": [{"keepalive": null, "name": "test", "remote-address": "1.2.3.4"}], "handle_absent_entries": "remove", "handle_entries_content": "remove", "path": "interface gre"}, "msg": "Key \"keepalive\" must not be disabled (value null/~/None) for name=\"test\"."}

2nd option: `'keepalive': KeyInfo(can_disable=True),`

Module isn't idempotent (when keepalive isn't specified):
The item is created the same way on the first run (with the default value coming from ROS).

But then it changes the value on the 2nd run:

TASK [mikrotik : Mikrotik api_modify] *******************************************************************************************************************************************************
Neděle 13 listopadu 2022  20:24:46 +0100 (0:00:01.358)       0:00:01.406 ****** 
--- before
+++ after
@@ -7,7 +7,6 @@
             "disabled": false,
             "dont-fragment": false,
             "dscp": "inherit",
-            "keepalive": "10s,10",
             "local-address": "0.0.0.0",
             "mtu": "auto",
             "name": "test",

changed: [new-konesin] => (item=interface gre)

Co-authored-by: Felix Fontein <[email protected]>

felixfontein · 2022-11-13T20:13:28Z

Ok, I understand now why it is necessary.

I want to to through the other code in more depth though to make sure we're not forgetting to change a place which would be affected by this as well.

therfert · 2022-11-13T20:15:52Z

Ok, I understand now why it is necessary.

I want to to through the other code in more depth though to make sure we're not forgetting to change a place which would be affected by this as well.

Thanks!

Signed-off-by: Tomas Herfert <herfik>

therfert · 2022-11-16T20:35:55Z

I added interface eoip path support into this PR as it requires the same feature for the keepalive field.

felixfontein · 2022-11-17T06:49:29Z

I think you need the following change as well if such paths should ever be used in the tests:

diff --git a/tests/unit/plugins/modules/fake_api.py b/tests/unit/plugins/modules/fake_api.py
index b17c088..f50c996 100644
--- a/tests/unit/plugins/modules/fake_api.py
+++ b/tests/unit/plugins/modules/fake_api.py
@@ -120,7 +120,7 @@ class Or(object):
 
 def _normalize_entry(entry, path_info):
     for key, data in path_info.fields.items():
-        if key not in entry and data.default is not None:
+        if key not in entry and data.default is not None and not data.can_disable:
             entry[key] = data.default
         if data.can_disable:
             if key in entry and entry[key] in (None, data.remove_value):

Besides that, it looks good to me!

Signed-off-by: Tomas Herfert <herfik>

therfert · 2022-11-17T07:44:09Z

@felixfontein done. btw. I also thought about the tests, but I had never done that and didn't really know where to start. (I'm not a developer :D )

felixfontein · 2022-11-17T11:49:53Z

Thanks again for yet another contribution :)

Tomas Herfert added 7 commits November 13, 2022 11:39

adding support for api fields that can be disabled and have default v…

e056bbb

…alue at the same time Signed-off-by: Tomas Herfert <herfik>

api path support: interface gre

65f9aa7

Signed-off-by: Tomas Herfert <herfik>

docs

498d345

Signed-off-by: Tomas Herfert <herfik>

unit test update & yamlling fix

514ff39

Signed-off-by: Tomas Herfert <herfik>

test fix

1b39357

Signed-off-by: Tomas Herfert <herfik>

sanity fix

a5f24a1

Signed-off-by: Tomas Herfert <herfik>

changelog

11fa41a

Signed-off-by: Tomas Herfert <herfik>

therfert marked this pull request as ready for review November 13, 2022 12:43

felixfontein reviewed Nov 13, 2022

View reviewed changes

plugins/module_utils/_api_data.py Outdated Show resolved Hide resolved

Update per suggestion

4f20cea

Co-authored-by: Felix Fontein <[email protected]>

felixfontein mentioned this pull request Nov 15, 2022

multiple api paths support and updates #131

Merged

Tomas Herfert added 2 commits November 16, 2022 21:31

api path support: interface eoip

014b09a

Signed-off-by: Tomas Herfert <herfik>

docs

7c40459

Signed-off-by: Tomas Herfert <herfik>

apply suggestion from code review

41bbf44

Signed-off-by: Tomas Herfert <herfik>

felixfontein approved these changes Nov 17, 2022

View reviewed changes

felixfontein merged commit 23b3aa3 into ansible-collections:main Nov 17, 2022

This was referenced Nov 18, 2022

Update Ansible collection community.routeros to v2.4.0 eyulf/homelab#257

Merged

feat(ansible-collection): update community.routeros to 2.5.0 - autoclosed anthr76/infra#1612

Closed

wyoa-bot bot mentioned this pull request Nov 18, 2022

feat(ansible): update community.routeros to 2.4.0 jr0dd/home-ops#1500

Merged

1 task

therfert deleted the api branch November 22, 2022 19:31

therfert mentioned this pull request Mar 17, 2023

ip dhcp-client: dhcp-options can be removed with value '' #154

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API update #128

API update #128

therfert commented Nov 13, 2022

codecov bot commented Nov 13, 2022 •

edited

Loading

github-actions bot commented Nov 13, 2022 •

edited

Loading

therfert commented Nov 13, 2022 •

edited

Loading

felixfontein left a comment

therfert commented Nov 13, 2022 •

edited

Loading

felixfontein commented Nov 13, 2022

therfert commented Nov 13, 2022

therfert commented Nov 16, 2022

felixfontein commented Nov 17, 2022

therfert commented Nov 17, 2022

felixfontein commented Nov 17, 2022

API update #128

API update #128

Conversation

therfert commented Nov 13, 2022

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

codecov bot commented Nov 13, 2022 • edited Loading

Codecov Report

github-actions bot commented Nov 13, 2022 • edited Loading

Docs Build 📝

therfert commented Nov 13, 2022 • edited Loading

felixfontein left a comment

Choose a reason for hiding this comment

therfert commented Nov 13, 2022 • edited Loading

Here you can see how api_modify works (with the proposed change):

Module parameters:

First run

Second run (module is idempotent):

disabling keepalive - new parameters:

First run

second run (module is idempotent)

current behaviour (without the proposed change)

1st option: 'keepalive': KeyInfo(default='10s,10')

2nd option: 'keepalive': KeyInfo(can_disable=True),

felixfontein commented Nov 13, 2022

therfert commented Nov 13, 2022

therfert commented Nov 16, 2022

felixfontein commented Nov 17, 2022

therfert commented Nov 17, 2022

felixfontein commented Nov 17, 2022

codecov bot commented Nov 13, 2022 •

edited

Loading

github-actions bot commented Nov 13, 2022 •

edited

Loading

therfert commented Nov 13, 2022 •

edited

Loading

therfert commented Nov 13, 2022 •

edited

Loading

1st option: `'keepalive': KeyInfo(default='10s,10')`

2nd option: `'keepalive': KeyInfo(can_disable=True),`