task288 - add dyndns tasks,test,handler #97

fnateghi · 2024-09-12T09:42:46Z

Pull Request Description:

This pull request introduces a new task for managing Dynamic DNS (DynDNS) settings in OPNsense, along with associated handlers and tests. The changes include the ability to remove and update general DynDNS settings, apply account-specific configurations, and restart the DynDNS service automatically. Additionally, comprehensive tests have been added to ensure proper configuration handling.

Key updates:

DynDNS task for applying general and account-specific settings.
New handler for restarting the DynDNS service.
Tests to validate XML configurations for DynDNS.

zerwes · 2024-09-12T09:47:57Z

closes #95

zerwes · 2024-09-12T10:04:28Z

tasks/dyndns.yml

+  delegate_to: localhost
+  community.general.xml:
+    path: "{{ local_config_path }}"
+    xpath: /opnsense/OPNsense/DynDNS{{ item }}


I would leave this a task for opn_unset
But if you prefer to do it in the dyndns task, you should just expect a list of UUIDs in the VAR and then use

xpath: "/opnsense/OPNsense/DynDNS/accounts/account[@uuid='{{ item }}']

to remove accounts

zerwes · 2024-09-12T10:21:51Z

test/test-dyndn.yml

Hello @fnateghi ... testing is great 👍
But ... Ough ... I think you have misunderstood the testing completely
You should test your task(s) and not write a complete playbook
That is why your wrong condition in line 40 in the task file was not breaking the test.
The test should not test himself, but should test your task and finde errors like the wrong condition above.

you must register your new test tag

diff --git a/test/test.yml b/test/test.yml index 2e8a39b..d071f55 100644 --- a/test/test.yml +++ b/test/test.yml @@ -44,6 +44,7 @@ - ipsec - dnsserver - openvpn + - dyndns when: - test | default(_testtask) == _testtask - ansible.builtin.meta: flush_handlers

and maybe add the missing handlere as a fake one here too
and then you define your VAR files for the tests as test/dyndns-test*.yml, optional some starting xml as test/dyndns-test*.xml and the expected result as test/dyndns-test*-expect.xml
and then you can run your test in the test directory using
ansible-playbook -e test=dyndns test.yml

zerwes · 2024-09-12T10:23:28Z

tasks/dyndns.yml

@@ -0,0 +1,41 @@
+---
+


adding a comment with a example of the VARs here would be great

zerwes · 2024-09-12T11:33:26Z

tasks/dyndns.yml

+    state: present
+    pretty_print: true
+  notify: register dyndns
+  loop: "{{ opn_dyndns_general }}"


and I would prefer to use `with_items' here
as in all other tasks

zerwes · 2024-09-12T12:45:21Z

tasks/dyndns.yml

+    - "{{ opn_dyndns_accounts }}"
+    - settings
+  when:
+    - opn_dyndns_accounts_ng is defined


- - opn_dyndns_accounts_ng is defined + - opn_dyndns_accounts is defined

…dler and add fake handler

zerwes · 2024-09-12T13:22:16Z

tasks/dyndns.yml

@@ -0,0 +1,77 @@
+---
+#
+#opn_dyndns_general:


pls fix the lint errors first ...

and the new dyndns-test should work pls

fnateghi · 2024-09-12T13:24:14Z

Dear @zerwes

Thank you for your prompt response and feedback. I encountered a small issue with my Git settings, which prevented me from committing the requested configuration initially. However, I have now modified the task and added the handler and the config example in the task as requested.

That said, I believe there might still be a minor issue in my code, though I'm not entirely sure what is wrong or where the problem lies. When I run the test playbook, I receive the following error message:

TASK [fail due to diff between result and expected state for dyndns-test] ****************************************************************************************************************** task path: /.../opnsense/test/testsimpletaskgeneric.yml:46 fatal: [localhost]: FAILED! => { "changed": false, "msg": "fail due to diff between result and expected state for dyndns-test" }

the result should be the enrire settings, but in the *expect.xml file i can only see the general part of the DynDNS settings. The account part is missing somehow.
Could you please assist me in identifying where I might have gone wrong?
thanks

zerwes · 2024-09-12T13:30:57Z

Dear @zerwes

Thank you for your prompt response and feedback. I encountered a small issue with my Git settings, which prevented me from committing the requested configuration initially. However, I have now modified the task and added the handler and the config example in the task as requested.

That said, I believe there might still be a minor issue in my code, though I'm not entirely sure what is wrong or where the problem lies. When I run the test playbook, I receive the following error message:

TASK [fail due to diff between result and expected state for dyndns-test] ****************************************************************************************************************** task path: /.../opnsense/test/testsimpletaskgeneric.yml:46 fatal: [localhost]: FAILED! => { "changed": false, "msg": "fail due to diff between result and expected state for dyndns-test" }

the result should be the enrire settings, but in the *expect.xml file i can only see the general part of the DynDNS settings. The account part is missing somehow. Could you please assist me in identifying where I might have gone wrong? thanks

Hello @fnateghi
I have already pointed you th the problem in the first review
#97 (comment)

zerwes · 2024-09-12T13:32:44Z

so it might be desirable to iterate all my comments in the code review one by one

fnateghi · 2024-09-12T13:50:39Z

Sorry @zerwes. I will fix this and commit again.

…al task

fnateghi · 2024-09-12T15:26:17Z

I hope this time is everything correct :)

zerwes

I hope this time is everything correct :)

nope, sadly not

zerwes · 2024-09-12T17:47:05Z

tasks/dyndns.yml

+    - settings
+  when:
+    - opn_dyndns_accounts is defined
+    - item.1.key != 'enable'


❓
why do you exclude the enable tag here? how do you handle it?

- - item.1.key != 'enable'

zerwes · 2024-09-12T17:53:01Z

test/test.yml

+    - name: restart dyndns
+      debug:
+        msg: fake handler - restart dyndns
+


pls, do not add empty lines where there was none was before ...

zerwes

test required, not a fake

test/dyndns-test.xml

zerwes

finally 👍

zerwes · 2024-09-13T08:12:26Z

due to the 2 steps back and one forward commits I will perform a squash merge to main

task288 - add dyndns tasks,test,handler

285dbea

fnateghi requested a review from zerwes September 12, 2024 10:11

zerwes requested changes Sep 12, 2024

View reviewed changes

zerwes self-assigned this Sep 12, 2024

zerwes added enhancement New feature or request test required labels Sep 12, 2024

zerwes reviewed Sep 12, 2024

View reviewed changes

add example vars into dyndnstask,add dyndns to test.yml, modified han…

45c9dac

…dler and add fake handler

zerwes reviewed Sep 12, 2024

View reviewed changes

fnateghi added 2 commits September 12, 2024 17:13

task288 - makes ansible-lint happy and corrected the dyndns.yml gener…

d9cc00a

…al task

task288 - fixt test error for dyndns

a0a00d8

fnateghi requested a review from zerwes September 12, 2024 15:29

zerwes requested changes Sep 12, 2024

View reviewed changes

test/dyndns-test.xml Show resolved Hide resolved

requested changes in dyndns.yml, dyndns-test.xml,test.yml

e61b1f6

zerwes approved these changes Sep 13, 2024

View reviewed changes

zerwes merged commit 05b1d8c into main Sep 13, 2024
4 checks passed

zerwes mentioned this pull request Sep 13, 2024

Add Missing Dynamic DNS Client Settings in OPNsense Role #95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

task288 - add dyndns tasks,test,handler #97

task288 - add dyndns tasks,test,handler #97

fnateghi commented Sep 12, 2024

zerwes commented Sep 12, 2024

zerwes Sep 12, 2024

zerwes Sep 12, 2024

zerwes Sep 12, 2024

zerwes Sep 12, 2024

zerwes Sep 12, 2024

zerwes Sep 12, 2024

zerwes Sep 12, 2024

fnateghi commented Sep 12, 2024

zerwes commented Sep 12, 2024

zerwes commented Sep 12, 2024

fnateghi commented Sep 12, 2024

fnateghi commented Sep 12, 2024

zerwes left a comment

zerwes Sep 12, 2024

zerwes Sep 12, 2024

zerwes Sep 12, 2024 •

edited

Loading

zerwes left a comment

zerwes left a comment

zerwes commented Sep 13, 2024

task288 - add dyndns tasks,test,handler #97

task288 - add dyndns tasks,test,handler #97

Conversation

fnateghi commented Sep 12, 2024

zerwes commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnateghi commented Sep 12, 2024

zerwes commented Sep 12, 2024

zerwes commented Sep 12, 2024

fnateghi commented Sep 12, 2024

fnateghi commented Sep 12, 2024

zerwes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zerwes Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

zerwes left a comment

Choose a reason for hiding this comment

zerwes left a comment

Choose a reason for hiding this comment

zerwes commented Sep 13, 2024

zerwes Sep 12, 2024 •

edited

Loading