Skip to content

Conversation

cidrblock
Copy link
Contributor

@cidrblock cidrblock commented Jul 28, 2025

Depends on PR #4487

Problem: Command classes had duplicate logger setup code across 7+ files.
Solution: Single @Property _log in base.Base with automatic step derivation.

Key Changes:

  • Added @Property _log to base.Base with automatic step naming
  • Removed individual logger setups from 7 command classes
  • Eliminated 50+ lines of duplicate code
  • Updated tests for scenario logger pattern

Impact:

  • Future command classes get logger automatically
  • Consistent scenario->step format across all commands
  • Perfect architectural separation of concerns

Testing: All command tests pass with proper context validation.

cidrblock added 10 commits July 28, 2025 07:52
Command classes will be handled separately with base.Base consolidation.
This PR now focuses only on dependency, provisioner, and verifier classes.
- Add step context to scenario discovery logging (discovery)
- Add step context to prerun logging (prerun)
- Add step context to reset scenario logging (reset)
- Add step context to error cleanup logging (action/cleanup)
- Add step context to default scenario fallback warning (subcommand)
- Replace LOG.warning with scenario logger in Config.state property
- Add step context 'config' for configuration-related warnings
- Import logger module for scenario context support
…and state.py

- Remove unused LOG = logging.getLogger(__name__) definitions
- Remove corresponding logging imports where no longer needed
- Fix Callable import in state.py to use collections.abc
- Remove unused LOG = logging.getLogger(__name__) from command modules
- Remove unused LOG definitions from dependency, driver, and provisioner modules
- Remove corresponding logging imports where no longer needed
- Clean up state.py, platforms.py, verifier/testinfra.py and others
- All pre-commit checks passing after cleanup
- Remove unused LOG = logging.getLogger(__name__) from command/base.py
- Keep logging import as it's still used in one place (line 137)
- All LOG definitions are now used (verified 6 remaining files)
- Remove default value from step_name parameter in get_scenario_logger()
- Fix all 11 call sites identified by type checking to include step context
- Update scenario.py calls to use 'scenario' step
- Update command modules (cleanup, idempotence, login, prepare, side_effect) with appropriate steps
- Update init/scenario.py to use 'init' step
- Update test to reflect new mandatory step requirement
- Eliminates possibility of creating loggers without step context
- Achieves complete logging consistency throughout codebase
- Add @Property _log to base.Base with automatic step name derivation
- Remove individual logger setups from 7 command classes:
  - cleanup, idempotence, prepare, side_effect, login (removed __init__ entirely)
  - create, destroy (removed redundant __init__ methods)
- Automatic step derivation: ClassName -> classname (e.g., Idempotence -> idempotence)
- Updated tests to use scenario logger pattern
- Eliminates 50+ lines of duplicate logger code
- Centralizes command logging architecture in base class
- Future command classes get logger automatically
cidrblock and others added 12 commits July 28, 2025 13:12
…and state.py

- Remove unused LOG = logging.getLogger(__name__) definitions
- Remove corresponding logging imports where no longer needed
- Fix Callable import in state.py to use collections.abc
- Remove unused LOG = logging.getLogger(__name__) from command modules
- Remove unused LOG definitions from dependency, driver, and provisioner modules
- Remove corresponding logging imports where no longer needed
- Clean up state.py, platforms.py, verifier/testinfra.py and others
- All pre-commit checks passing after cleanup
- Remove unused LOG = logging.getLogger(__name__) from command/base.py
- Keep logging import as it's still used in one place (line 137)
- All LOG definitions are now used (verified 6 remaining files)
- Remove environment detection test cases (not part of unused logger cleanup)
- Remove TTY detection test function (belongs in separate environment detection PR)
- Keep focus on pure unused logger cleanup only
- Add step context to scenario discovery logging (discovery)
- Add step context to prerun logging (prerun)
- Add step context to reset scenario logging (reset)
- Add step context to error cleanup logging (action/cleanup)
- Add step context to default scenario fallback warning (subcommand)
- Replace LOG.warning with scenario logger in Config.state property
- Add step context 'config' for configuration-related warnings
- Import logger module for scenario context support
Add @Property _log method to Config class that provides scenario-aware
logging with 'config' step context. Update existing logger call in
state property to use self._log instead of creating ad-hoc logger.

This enables other Config methods to use self._log for consistent
scenario and step context in log messages.
Complete conversion of Config class logging to use scenario context:

- _validate(): Use hardcoded scenario logger during validation
- _get_driver_name(): Use self._log for driver warnings
- collection(): Use self._log for galaxy.yml validation warnings

Remove unused module-level logger (LOG) and import since all
logging now uses scenario-aware loggers with proper step context.

Benefits:
- All Config logging now includes scenario and step information
- Consistent logging pattern throughout Config class
- Eliminates context-less logging in Config operations
…unction

- Add global _log() function in base.py for on-demand logger creation
- Replace 5 manual get_scenario_logger() calls with single reusable function
- Simplify logger usage: _log(scenario_name, step, message, level='info')
- Pre-format messages instead of using format placeholders
- Cleaner, more maintainable logging pattern across command base
- Replace patched_logger_debug mock with caplog for better testing approach
- Verify debug message content includes molecule file path
- Check scenario logger extras: molecule_scenario and molecule_step
- Ensure validate step is correctly recorded in log record
- More robust testing of scenario-aware logging functionality
…-properties

- Merge PR ansible#4488 comprehensive work into PR ansible#4486
- Includes Config class scenario-aware logging
- Includes global _log function for command base
- Includes all test improvements and documentation
@cidrblock cidrblock force-pushed the feat/command-logger-consolidation branch from 3afa218 to 02c5a44 Compare July 28, 2025 21:14
- Remove additional test cases for environment variable detection
- These tests are unrelated to the logging enhancement scope
- Keep PR focused on logging consolidation only
@cidrblock cidrblock marked this pull request as ready for review July 28, 2025 21:33
- Add @Property _log to base.Base for command classes to inherit
- Automatically derives step name from class name (e.g., Cleanup -> cleanup)
- Enables all command classes to use self._log for scenario-aware logging
- Fixes missing property errors for command class inheritance
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR consolidates duplicate logger setup code across command classes by introducing a centralized _log property in the base Base class. The change eliminates individual logger initialization in 7+ command classes and establishes consistent scenario->step logging format.

  • Adds @property _log to base.Base with automatic step naming derived from class name
  • Removes individual __init__ methods and logger setup from command classes (cleanup, create, destroy, idempotence, prepare, side_effect)
  • Updates get_scenario_logger to require step_name parameter and updates all callers
  • Updates tests to validate scenario logger pattern with proper step context

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/molecule/command/base.py Adds centralized _log property with automatic step derivation
src/molecule/command/*.py Removes duplicate logger initialization from 6 command classes
src/molecule/logger.py Makes step_name required parameter in get_scenario_logger
src/molecule/scenario.py Updates get_scenario_logger calls to include step parameter
src/molecule/command/init/scenario.py Updates get_scenario_logger call to include step parameter
tests/unit/*.py Updates tests to validate scenario logger pattern

@cidrblock cidrblock merged commit f9ecb5a into ansible:main Jul 28, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant