Skip to content
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.

Sign up for GitHub

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

Fix typo & Fix app_id requirement and default #124

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Fix typo & Fix app_id requirement and default #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7e6f2f8
Fix typo
StevenH1901 Jan 10, 2025
693730d
Update app_id to be not required and set default to ansible as per do…
StevenH1901 Jan 10, 2025
57b543c
list_order type should be int, not bool
StevenH1901 Jan 10, 2025
f2aecf3
Typo fix
StevenH1901 Jan 10, 2025
15ebb17
Merge branch 'develop' into develop
cmeissner Jan 12, 2025
758ed3f
Test section list_order works
StevenH1901 Jan 15, 2025
bb583bd
New test to ensure app_id deafults to 'ansible'
StevenH1901 Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/plugins/address_module.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ Parameters

<div class="ansible-option-cell">

IP address to hanle
IP address to handle


.. raw:: html
Expand Down
2 changes: 1 addition & 1 deletion plugins/module_utils/phpipam_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def __init__(self, **kwargs):
self.phpipam_spec, gen_args = self._phpipam_spec_helper(kwargs.pop('phpipam_spec', {}))
argument_spec = dict(
server_url=dict(required=True, fallback=(env_fallback, ['PHPIPAM_SERVER_URL'])),
app_id=dict(required=True, fallback=(env_fallback, ['PHPIPAM_APP_ID'])),
app_id=dict(required=False, fallback=(env_fallback, ['PHPIPAM_APP_ID']), default='ansible'),
username=dict(required=True, fallback=(env_fallback, ['PHPIPAM_USERNAME'])),
password=dict(required=True, fallback=(env_fallback, ['PHPIPAM_PASSWORD']), no_log=True),
validate_certs=dict(type='bool', fallback=(env_fallback, ['PHPIPAM_VALIDATE_CERTS']), required=False, default=True),
Expand Down
4 changes: 2 additions & 2 deletions plugins/modules/section.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,11 @@ def main():
phpipam_spec=dict(
name=dict(type='str', required=True),
description=dict(type='str', required=False),
parent=dict(type='entity', controller='sections', required=False, defautl=None, phpipam_name='masterSection'),
parent=dict(type='entity', controller='sections', required=False, default=None, phpipam_name='masterSection'),
permissions=dict(type='json', required=False, default=None),
strict_mode=dict(type='bool', required=False),
subnet_ordering=dict(type='bool', required=False, phpipam_name='subnetOrdering'),
list_order=dict(type='bool', required=False, phpipam_name='order'),
list_order=dict(type='int', required=False, phpipam_name='order'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From your test, I noticed that list_order accepts values from 1 to 3. Are there corresponding strings used in the UI?
Maybe you can point to the correct point in the UI. If there are strings in the UI, we need to provide it here to work with these words instead of int. And in background, we need to translate these strings to int's for talking to the API.

Copy link
Member

@cmeissner cmeissner Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

From the section edit screen, I can find these Options to select. From this finding, two things came to my mind:

  1. Obviously, we need a combination of a order_by and how to order (asc, desc), to make it human-readable.
  2. Do we need to set up an obvious UI-only used property from Ansible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the result from the API, if I queried a section

$ curl -sk -XGET -H "phpipam-token: ${PHPIPAM_TOKEN}" ${PHPIPAM_API_URL}/sections/2 | jq
{
  "code": 200,
  "success": true,
  "data": {
    "id": "2",
    "name": "IPv6",
    "description": "Section for IPv6 addresses",
    "masterSection": "0",
    "permissions": "{\"2\":\"2\",\"3\":\"1\"}",
    "strictMode": "1",
    "subnetOrdering": "subnet,asc",
    "order": null,
    "editDate": "2025-01-22 14:55:26",
    "showVLAN": "0",
    "showVRF": "0",
    "showSupernetOnly": "0",
    "DNS": null
  },
  "time": 0.003
}

subnetOrdering is corresponding with the one I found in the UI. For order which corresponds with list_order, I was unable to find a UI element. Can you please guide me to the corresponding UI element to check different values available for that property?

On the other hand, I recognized that we do not handle subnetOrdering with this module and here we can open a follow-up issue to track and implement it later.

show_vlan=dict(type='bool', required=False, phpipam_name='showVLAN'),
show_vrf=dict(type='bool', required=False, phpipam_name='showVRF'),
show_supernets_only=dict(type='bool', required=False, phpipam_name='showSupernetOnly'),
Expand Down
14 changes: 14 additions & 0 deletions tests/test_playbooks/shared_vars.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
- name: Shared vars test
hosts: localhost
gather_facts: false
vars_files:
- vars/server.yml
- vars/address.yml
tasks:
- name: Check app_id defaults to 'ansible'
ansible.builtin.include_tasks: tasks/address.yml
vars:
override:
phpipam_app_id: ""
step: create address again, no change
address: '{{ base_address_data }}'
3 changes: 3 additions & 0 deletions tests/test_playbooks/vars/section.yml
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to put list_order in base_section_data to have it tested in test-section.

If you do so, the test shows no idempotency anymore. Please investigate further.

Copy link
Member

@cmeissner cmeissner Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to split the list_order issue into another new issue. Cherry-pick the commits you made so far into another branch and remove them from this branch.

So we only handle here the app_id issue and the two typos.

I expect that the list_order issue would become greater, because of missing idempotency and the confusion between oder and list_order.

Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ base_section_data:

sections:
- name: "ACME Inc."
list_order: 3
- name: "DevOps Department"
parent: "ACME Inc."
list_order: 1
- name: "Infrastructure Department"
parent: "ACME Inc."
list_order: 2