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.

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

Add Icinga Deploy handler and module #205

Merged
merged 18 commits into from
May 22, 2023

Conversation

flkhndlr
Copy link
Contributor

This PR adds a module and handler to trigger a deployment of the Icinga config. Tasks are updated to call the handler.
The default behavior is not changed, so that current users are not confronted with changes in execution.

I'm not sure how to integrate testing for the handler and how to document the usage in a correct way.
Thanks in advance.

This PR resolves Part of #90.

@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Merging #205 (5f7e296) into master (cdcb8a6) will decrease coverage by 1.25%.
The diff coverage is 74.19%.

@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
- Coverage   97.25%   96.01%   -1.25%     
==========================================
  Files          41       43       +2     
  Lines        1094     1154      +60     
  Branches      171      180       +9     
==========================================
+ Hits         1064     1108      +44     
- Misses         16       28      +12     
- Partials       14       18       +4     
Flag Coverage Δ
integration 92.52% <70.96%> (-1.24%) ⬇️
sanity 53.37% <48.38%> (-0.28%) ⬇️

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

Impacted Files Coverage Δ
plugins/modules/icinga_deploy.py 65.38% <65.38%> (ø)
plugins/module_utils/icinga.py 88.37% <76.92%> (-1.46%) ⬇️
plugins/modules/icinga_deploy_info.py 82.60% <82.60%> (ø)

@flkhndlr flkhndlr marked this pull request as ready for review May 4, 2023 11:08
RETURN = r""" # """

from ansible.module_utils.urls import url_argument_spec
from ansible.module_utils.basic import AnsibleModule
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from ansible.module_utils.basic import AnsibleModule
from ansible.module_utils.basic import AnsibleModule

# Define the main module
module = AnsibleModule(
argument_spec=argument_spec,
supports_check_mode=True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
supports_check_mode=True,
supports_check_mode=False,

Comment on lines 26 to 34
module: icinga_deploy
short_description: Trigger deployment in Icinga2
description:
- Trigger a deployment to Icinga2 through the director API.
author: Falk Händler (@flkhndlr)
version_added: '1.33.0'
extends_documentation_fragment:
- ansible.builtin.url
- t_systems_mms.icinga_director.common_options
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
module: icinga_deploy
short_description: Trigger deployment in Icinga2
description:
- Trigger a deployment to Icinga2 through the director API.
author: Falk Händler (@flkhndlr)
version_added: '1.33.0'
extends_documentation_fragment:
- ansible.builtin.url
- t_systems_mms.icinga_director.common_options
module: icinga_deploy_info
short_description: Query deployment information in Icinga2
description:
- Get deployment information through the director API.
author: Falk Händler (@flkhndlr)
version_added: '1.33.0'
extends_documentation_fragment:
- ansible.builtin.url
- t_systems_mms.icinga_director.common_options

"""

RETURN = r"""
config:
Copy link
Collaborator

Choose a reason for hiding this comment

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

When querying the API, the director returns an object called active_configuration. Let's try to stick to this. So I propose to rename config to active_configuration.

Comment on lines 72 to 73
query=dict(type="str", required=False, default=""),
resolved=dict(type="bool", default=False, choices=[True, False]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

These seem to be unused.

Suggested change
query=dict(type="str", required=False, default=""),
resolved=dict(type="bool", default=False, choices=[True, False]),

url_username: "{{ icinga_user }}"
url_password: "{{ icinga_pass }}"
when: icinga_deploy_config and icinga_deploy_config is defined
run_once: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Handlers always run once.

Suggested change
run_once: true

url_username: "{{ icinga_user }}"
url_password: "{{ icinga_pass }}"
when: icinga_deploy_config and icinga_deploy_config is defined
run_once: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Handlers always run once.

Suggested change
run_once: true

object_list = icinga_object.query()

module.exit_json(
config=object_list["data"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now we return an object called config that contains a dict called active_configuration that then contains the actual data:

ok: [localhost] => {"changed": false, "config": {"active_configuration": {"activity": "a4c955364bc7b77efd0323fc87d95307f827e30c", "config": "b175ca0562434deeb4fb1fc03fd80cd7361b56df", "stage_name": "5e4ee411-dff9-
461e-b530-a6da21cc070e"}}}

Let's try to remove that indirection by directly returning the contents of active_configuration. We can achieve that by doing this:

Suggested change
config=object_list["data"],
config=object_list["data"]["active_configuration"],

Then we get back this:

ok: [localhost] => {"active_configuration": {"activity": "a4c955364bc7b77efd0323fc87d95307f827e30c", "config": "b175ca0562434deeb4fb1fc03fd80cd7361b56df", "stage_name": "5e4ee411-dff9-461e-b530-a6da21cc070e"}, "changed": false}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a good idea, but one thing I noticed was, that the automatic testgeneration wants "objects" as the key. So I changed it to objects rather than active_configuration. If we want to update the testgeneration, I'm open to it :)

@rndmh3ro rndmh3ro added enhancement New feature or request minor labels May 22, 2023
@rndmh3ro rndmh3ro merged commit 62227a7 into telekom-mms:master May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants