Add Log class and logging config by allenrobel · Pull Request #184 · CiscoDevNet/ansible-nd

allenrobel · 2026-03-02T18:30:30Z

Summary

Adds the Log class for configuring Python logging across the ND collection. This PR is one of two companion PRs targeting nd42_integration:

This PR nd42_logging — Log class, logging config, and test_log.py
Add RestSend framework, enums, and shared unit test infrastructure #185 nd42_rest_send — RestSend framework, enums.py, and all shared test infrastructure

Note: test_log.py imports from common_utils and enums which reside in nd42_rest_send (#185). Tests will not run until both branches are merged into nd42_integration.

Files added

File	Description
`plugins/module_utils/log.py`	`Log` class — configures Python logging from an external JSON `dictConfig` file; validates that only file-based handlers are used (no stdout/stderr interference with Ansible)
`plugins/module_utils/logging_config.json`	Ready-to-use `logging.config.dictConfig`-compatible config writing to `/tmp/nd.log` via `RotatingFileHandler`
`tests/unit/test_log.py`	Unit tests for the `Log` class

Log class behaviour

Reads ND_LOGGING_CONFIG env var to locate the logging config JSON file
Validates that the config contains only file-based handlers to avoid interfering with Ansible's JSON output
Calls logging.config.dictConfig() to configure the root nd logger
Disables logging entirely (adds NullHandler) when no config is provided
develop property controls whether the logging module raises internal exceptions (default: False)

Merge order

Merge nd42_rest_send (Add RestSend framework, enums, and shared unit test infrastructure #185) → nd42_integration (provides enums.py and shared test infrastructure)
Merge this PR → nd42_integration

Test plan

After merging both PRs into nd42_integration: python -m pytest tests/unit/test_log.py passes cleanly
tox -e linters passes (black, isort, pylint, mypy)
ansible-test sanity --docker passes

🤖 Generated with Claude Code

- plugins/module_utils/log.py: Log class for configuring Python logging across the ND collection via an external JSON DictConfig file - plugins/module_utils/logging_config.json: Ready-to-use logging config writing to /tmp/nd.log via RotatingFileHandler - tests/unit/test_log.py: Unit tests for the Log class Note: test_log.py imports from the shared test infrastructure (common_utils, enums, etc.) which resides in the nd42_rest_send branch. Tests will not run until both branches are merged into nd42_integration. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

plugins/module_utils/common/log.py

plugins/module_utils/logging_config.json

plugins/module_utils/log.py

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Ansible sanity apparently does not like the docstring above the from __future__ import.

It could be that Ansible sanity is using a regex to exactly match the import below. Trying to see if we can break this into two imports. ERROR: Found 1 future-import-boilerplate issue(s) which need to be resolved: ERROR: plugins/module_utils/log.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

We may not need annotations since our type hints already import Optional. Let me try removing it.

- Move plugins/module_utils/log.py to plugins/module_utils/common/log.py - Add setup_logging() helper to simplify logging setup in Ansible modules - Update import paths in test_log.py and docstring examples - Add tests/unit package __init__.py files - Add unit tests for setup_logging() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ValidLogHandlers enum now holds handler class names rather than key names, so validation correctly rejects console/stream handlers regardless of what the user names them, and accepts file handlers with non-standard names. Added tests 00231 and 00232 to cover both previously broken cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Adding for Ansible sanity tests

tests/unit/test_log.py

plugins/module_utils/logging_config.json

samiib · 2026-03-06T01:48:36Z

tests/unit/test_log.py

@@ -0,0 +1,569 @@
+# -*- coding: utf-8 -*-


Since this may be one of the first set of unit tests created for this repo, the Unit test CI step can now be enabled. If it is working in this branch already it can be turned on in this PR or later in the nd42_integrated branch.
Edit:
As the note mentions it will only work in nd42_integrated, please enable the Unit test step when all is merged.

@samiib should we track CI step in a separate issue? Do you want to create an issue for this?

@samiib lets open a separate PR again nd42_integrated that turns on unit tests.

Thanks @samiib

👍 I can enable the step when the PRs are merged together. #196

akinross · 2026-03-06T10:10:18Z

plugins/module_utils/common/log.py

+        logging.raiseExceptions = False
+
+        self._config: Optional[str] = environ.get("ND_LOGGING_CONFIG", None)
+        self._develop: bool = False


I was looking at the _develop attribute and have a short question to clarify my understanding. Is my understanding correct that this attribute is used to prevent logging failures to be the reason that a module fails?

This has no importance now, but do you think as a future enhancement we should track displaying a warning to the ansible user that logs are being ignored for whatever reason? Or should we accept that logging would silently ignore and user should be responsible for having appropriate logging capabilities like disk space and permissions set etc?

@akinross

Yes, the _develop attribute disables any exceptions coming from the Python logging module. If set to True, and logging itself raises an exception, this gets passed to us to handle.

And, yes, I think, it would be good to alert the user when _develop == True so would agree that an issue should be raised to track adding this. @mikewiebe the warning would probably be via an Action plugin, right?

@allenrobel Not sure if we could use an action plugin for this. This gets initialized in the module when log = Log() is called right? In that case it would be to late to alert the user in an action plugin.

akinross · 2026-03-06T10:31:51Z

tests/unit/test_log.py

@@ -0,0 +1,569 @@
+# -*- coding: utf-8 -*-


@samiib should we track CI step in a separate issue? Do you want to create an issue for this?

plugins/module_utils/common/log.py

lhercot · 2026-03-06T15:11:21Z

tests/unit/test_log.py

@@ -0,0 +1,569 @@
+# -*- coding: utf-8 -*-


@samiib lets open a separate PR again nd42_integrated that turns on unit tests.

tests/unit/test_log.py

plugins/module_utils/common/log.py

Co-authored-by: Lionel Hercot <lhercot@cisco.com>

@samiib

Good catch @samiib

@samiib

Good catch @samiib

Allows callers to pass config path and develop flag directly to the constructor while preserving property setter validation for both. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Convert all docstrings from ### heading style to standard # Summary / ## sections, replace double-backtick references with single backticks, and remove premature line wraps to conform to the 159-character line length limit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Four categories of issues were addressed: 1. Test isolation (env var leakage between tests) All tests that assigned directly to environ["ND_LOGGING_CONFIG"] did not clean up afterward. Tests relying on the env var being absent (e.g. test_log_00100) could pass for the wrong reason depending on execution order. Fix: replaced all direct environ[...] assignments with monkeypatch.setenv() (pytest's built-in fixture that automatically restores env state after each test). Added monkeypatch.delenv("ND_LOGGING_CONFIG", raising=False) to every test that requires the env var to be absent. Removed the now-unused "from os import environ" import. Affected tests: test_log_00010, test_log_00100, test_log_00110, test_log_00120, test_log_00200, test_log_00210, test_log_00220, test_log_00230, test_log_00231, test_log_00232, test_log_00240, test_log_00250, test_setup_logging_00010, test_setup_logging_00020. 2. Missing initialization test (test_log_00000) No test verified the default state of Log() when ND_LOGGING_CONFIG is not set. Added test_log_00000 to assert instance.config is None, instance.develop is False, and logging.raiseExceptions is False. 3. Missing constructor parameter tests (test_log_00020, test_log_00030) The Log(config=...) and Log(develop=True) constructor paths were never exercised directly; only the property setters were tested. - test_log_00020: passes config= directly to Log(), calls commit(), and verifies messages appear in the log file without setting ND_LOGGING_CONFIG. - test_log_00030: passes develop=True to Log() and asserts both instance.develop is True and logging.raiseExceptions is True. 4. Missing property/setter tests - test_log_00130: verifies that setting instance.config to a different file path after construction overrides ND_LOGGING_CONFIG; confirms messages appear in the new file and not the env-var file. - test_log_00320 (parametrized True/False): verifies the develop setter side effect -- that logging.raiseExceptions is updated to match the value set. test_log_00310 already verified instance.develop == develop but never checked the raiseExceptions side effect at setter line 404 of log.py. - test_log_00233/00234/00235: ValidLogHandlers defines four accepted handler classes; only RotatingFileHandler was covered. Added one test per remaining class (FileHandler, TimedRotatingFileHandler, WatchedFileHandler) to confirm commit() succeeds for each. - test_setup_logging_00030: setup_logging() with ND_LOGGING_CONFIG absent should return a Log instance with logging disabled without calling fail_json. No test covered this path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- setup_logging() now accepts develop: bool = False, passing it through to Log() so callers can enable logging exceptions without bypassing the helper. - Updated Log class docstring Usage section to recommend setup_logging() as the primary approach, with direct Log() usage shown as an alternative. - Added test_setup_logging_00040 (parametrized True/False) to verify develop propagates correctly through setup_logging(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

plugins/module_utils/common/log.py

lhercot

LGTM

akinross

LGTM

samiib

LGTM

shrsr · 2026-03-09T13:48:07Z

plugins/module_utils/common/log.py

+    try:
+        log = Log(develop=develop)
+        log.commit()
+    except ValueError as error:


Shall we take care of this or is this not required?->
The develop parameter has a type hint of bool and a default value of False. Since Python does not enforce type hints at runtime, if a caller passes a non boolean value (eg develop="yes"),
it flows into init() which calls the develop setter. The setter validates the type with isinstance() and raises TypeError for non boolean values. However, this except block only catches ValueError, so the TypeError propagates up as an unhandled exception, bypassing fail_json().

@allenrobel My apologies for the last minute comment. Everything else looks good!

Will merge the current PR and we should address this in a subsequent PR that also remove the need for fail_json to adhere to the placement of it only in modules main()

Will be addressed in subsequent PR against integration branch

allenrobel requested review from akinross, anvitha-jain, gmicol, lhercot, sajagana, samiib and shrsr as code owners March 2, 2026 18:30

allenrobel mentioned this pull request Mar 2, 2026

Add RestSend framework, enums, and shared unit test infrastructure #185

Open

3 tasks

allenrobel force-pushed the nd42_logging branch from 2396cf6 to c736b77 Compare March 2, 2026 23:53

allenrobel changed the title ~~Add Log class, logging config, and unit tests~~ Add Log class and logging config Mar 2, 2026

allenrobel mentioned this pull request Mar 3, 2026

Add Smart Endpoints framework for ND API v1 #186

Open

3 tasks

akinross reviewed Mar 3, 2026

View reviewed changes

plugins/module_utils/common/log.py Show resolved Hide resolved

plugins/module_utils/logging_config.json Show resolved Hide resolved

akinross reviewed Mar 4, 2026

View reviewed changes

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

This was referenced Mar 4, 2026

CR: Initialize logging with single line in module, without try/except block. #190

Closed

CR: Leverage enums for valid_handlers in logger #191

Closed

akinross requested changes Mar 4, 2026

View reviewed changes

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

allenrobel and others added 9 commits March 4, 2026 10:10

Remove unused inspect import

f57e258

Replace valid_handlers set with ValidLogHandlers enum

080f4b3

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Remove module docstring to resolve sanity error

12bb032

Ansible sanity apparently does not like the docstring above the from __future__ import.

Remove annotations from import __future__

2a20dfe

We may not need annotations since our type hints already import Optional. Let me try removing it.

Add tests/sanity/config.yml

9e1e155

Adding for Ansible sanity tests

Move config.yml under tests/

b4544be

samiib requested changes Mar 6, 2026

View reviewed changes

akinross reviewed Mar 6, 2026

View reviewed changes

lhercot requested changes Mar 6, 2026

View reviewed changes

Apply suggestion from @lhercot

425461d

Co-authored-by: Lionel Hercot <lhercot@cisco.com>

mikewiebe and others added 7 commits March 6, 2026 10:54

Apply suggestions from code review

340207f

Co-authored-by: Lionel Hercot <lhercot@cisco.com>

Replace “dcnm” with “nd” everrywhere

86a567a

Good catch @samiib

Replace “dcnm” with “nd” everrywhere

fd1f715

Good catch @samiib

Add config and develop params to Log.__init__()

4624a08

Allows callers to pass config path and develop flag directly to the constructor while preserving property setter validation for both. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

lhercot requested changes Mar 6, 2026

View reviewed changes

plugins/module_utils/common/log.py Show resolved Hide resolved

samiib mentioned this pull request Mar 8, 2026

Enable unit test step in CI (DCNE-692) #196

Open

akinross self-requested a review March 8, 2026 07:59

samiib self-requested a review March 8, 2026 10:10

lhercot approved these changes Mar 9, 2026

View reviewed changes

akinross mentioned this pull request Mar 9, 2026

CR: Document Logger Functionality #189

Open

akinross approved these changes Mar 9, 2026

View reviewed changes

samiib approved these changes Mar 9, 2026

View reviewed changes

mtarking approved these changes Mar 9, 2026

View reviewed changes

mikewiebe approved these changes Mar 9, 2026

View reviewed changes

shrsr previously requested changes Mar 9, 2026

View reviewed changes

lhercot merged commit 916bef6 into nd42_integration Mar 9, 2026
29 of 30 checks passed

lhercot deleted the nd42_logging branch March 9, 2026 16:52

Conversation

allenrobel commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Files added

Log class behaviour

Merge order

Test plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samiib Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akinross Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allenrobel Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lhercot left a comment

Choose a reason for hiding this comment

Uh oh!

akinross left a comment

Choose a reason for hiding this comment

Uh oh!

samiib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

allenrobel commented Mar 2, 2026 •

edited

Loading

samiib Mar 6, 2026 •

edited

Loading

akinross Mar 6, 2026 •

edited

Loading

allenrobel Mar 6, 2026 •

edited

Loading