Update datadog downtime module with datadog API v2 (#9080) #9098

gmaOCR · 2024-11-03T19:02:10Z

SUMMARY

Global review to add the asked feature and match with the new version 2.29 of datadog API
Include "Fixes #9080 "
ISSUE TYPE

Feature Pull Request
Refactoring Pull Request

ADDITIONAL INFORMATION

Module req: python3.7+
Test for older version are skipped

@pilou-
@misc

…ns#9080)

ansibullbot · 2024-11-03T19:02:46Z

cc @DataDog
click here for bot help

russoz

hi @gmaOCR,

Thank you for the contribution! Please add a changelog fragment.

Also, this PR needs some TLC. See comments.

russoz · 2024-11-03T20:08:43Z

plugins/modules/datadog_downtime.py

-__metaclass__ = type
-


This should be kept. See https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_general.html#creating-a-module

russoz · 2024-11-03T20:30:40Z

plugins/modules/datadog_downtime.py

+            state=dict(required=False, choices=[
+                       "present", "absent"], default="present"),


I would recommend removing these formatting changes and submitting them on a separate PR. It takes longer for us to analyse the request as we need to verify for each block of lines changed whether it was just reformatting or new logic.

And the max length for lines in this code is meant to be 160 (sanity tests will complain if past that).

russoz · 2024-11-03T20:39:26Z

plugins/modules/datadog_downtime.py

+        if ka != kb:
+            return False
+        for k in ka:
+            if not _equal_dicts(a[k], b[k], ignore_keys):
+                return False
+        return True


Woldn't it be easier to just change it to:

Suggested change

if ka != kb:

return False

for k in ka:

if not _equal_dicts(a[k], b[k], ignore_keys):

return False

return True

return ka == kb and all(_equal_dicts(a[k], b[k], ignore_keys) for k in ka)

russoz · 2024-11-03T20:45:55Z

plugins/modules/datadog_downtime.py

+    elif isinstance(a, list) and isinstance(b, list):
+        if len(a) != len(b):
+            return False
+        return all(_equal_dicts(x, y, ignore_keys) for x, y in zip(a, b))
+
+    else:
+        return a == b


Not sure about this one:

In the entire module, it seems _equal_dicts() is called from only one place (other than the recursion) in line 333 and that code seems to ensure the data type is dict rather than list. Providing for other types is redundant.

The name of the function is _equal_dicts() - if we are comparing list and , the name is wrong

It seems like an unnecessary complication.

russoz · 2024-11-03T20:47:29Z

plugins/modules/datadog_downtime.py


+        module.fail_json(msg="Failed to update downtime: %s" % (e,))


Why moving from .format() to % formatting?

russoz · 2024-11-03T20:56:24Z

plugins/modules/datadog_downtime.py

+        monitor_id: null
+        monitor_tags:
+            - "foo:bar"
+        parent_id: null
+        recurrence: null
+        scope:
+            - "test"
+        start: 1607015009
+        timezone: "UTC"
+        updater_id: null
+        monitor_identifier:
+            monitor_id: 12345


Lines 151 and 161-162 seem to be redundant. Will monitor_identifier bring anything else other than monitor_id? If not, then maybe we should keep monitor_id as top level and not add ambiguity.

russoz · 2024-11-03T20:58:25Z

plugins/modules/datadog_downtime.py

            monitor_id=dict(required=False, type="int"),
+            monitor_identifier=dict(required=False, type="dict"),


Again, if monitor_identifier only brings the information monitor_id, then I see no point in adding that parameter (as we already have monitor_id).

As a matter of fact, it seems this new parameter is never used. When building the data payload for the API, it uses monitor_id directly.

felixfontein

Thanks for your contribution!

Please note that changes to a module must be backwards compatible. Existing (successful) uses of the module must continue to work, otherwise a change will not get merged.

I know next to nothing on datadog, like whether you can have an older on-prem version or whether there's only the SaaS option; and which client versions are still supported by versions of the software that are running (usually easier to answer if there's no on-prem solution).

If the current module can still be used with an older version of the client on Python 3.6, then the module needs to keep supporting that. To drop support for something, it needs to be deprecated first for a certain amount of time (roughly ~6 months+).

felixfontein · 2024-11-10T12:56:47Z

plugins/modules/datadog_downtime.py

-        elements: str
+          - A scope to which the downtime applies.
+          - For example, 'host:app2' or 'env:staging'
+        type: str


Changing the type from list of strings to string is a breaking change that prevents this PR from being merged.

felixfontein · 2024-11-10T12:57:04Z

plugins/modules/datadog_downtime.py

 author:
  - Datadog (@Datadog)
 requirements:
  - datadog-api-client
-  - Python 3.6+
+  - Python 3.7+


This is also a breaking change.

Update datadog downtime module with datadog API v2 (ansible-collectio…

d69f329

…ns#9080)

ansibullbot added WIP Work in progress feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests unit tests/unit labels Nov 3, 2024

gmaOCR marked this pull request as ready for review November 3, 2024 19:06

ansibullbot removed the WIP Work in progress label Nov 3, 2024

felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label Nov 3, 2024

russoz requested changes Nov 3, 2024

View reviewed changes

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 3, 2024

felixfontein added the backport-10 Automatically create a backport for the stable-10 branch label Nov 4, 2024

felixfontein reviewed Nov 10, 2024

View reviewed changes

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update datadog downtime module with datadog API v2 (#9080) #9098

Update datadog downtime module with datadog API v2 (#9080) #9098

gmaOCR commented Nov 3, 2024

ansibullbot commented Nov 3, 2024

russoz left a comment

russoz Nov 3, 2024

russoz Nov 3, 2024

russoz Nov 3, 2024

russoz Nov 3, 2024

russoz Nov 3, 2024

russoz Nov 3, 2024

russoz Nov 3, 2024

felixfontein left a comment

felixfontein Nov 10, 2024

felixfontein Nov 10, 2024

		state=dict(required=False, choices=[
		"present", "absent"], default="present"),


		module.fail_json(msg="Failed to update downtime: %s" % (e,))

		monitor_id=dict(required=False, type="int"),
		monitor_identifier=dict(required=False, type="dict"),

		__metaclass__ = type

Update datadog downtime module with datadog API v2 (#9080) #9098

Are you sure you want to change the base?

Update datadog downtime module with datadog API v2 (#9080) #9098

Conversation

gmaOCR commented Nov 3, 2024

ansibullbot commented Nov 3, 2024

russoz left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfontein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment