-
Notifications
You must be signed in to change notification settings - Fork 669
Consolidate command class loggers into base.Base property #4486
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
Consolidate command class loggers into base.Base property #4486
Conversation
…er and command classes
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
…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
3afa218
to
02c5a44
Compare
- Remove additional test cases for environment variable detection - These tests are unrelated to the logging enhancement scope - Keep PR focused on logging consolidation only
- 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
There was a problem hiding this 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
tobase.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 |
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:
Impact:
Testing: All command tests pass with proper context validation.