Skip to content

Conversation

cidrblock
Copy link
Contributor

@cidrblock cidrblock commented Jul 28, 2025

Problem: Several core components (dependency, provisioner, verifier, and commands) cache their loggers at initialization, preventing them from picking up dynamic scenario and step context during execution.

Solution: Converted self._log from instance variable to @Property in multiple classes:

  • Dependency (base, shell, ansible-galaxy)
  • Provisioner (ansible, playbook, playbooks)
  • Verifier (ansible, testinfra)
  • Commands (create, destroy)

Each property now dynamically retrieves fresh scenario and step context on access using getattr(self._config, 'action', default_step).

Impact: Log messages from these components now consistently show the current scenario and step context in the format 'scenario ➜ step: message'.

Testing: Verified via integration tests across multiple scenarios and commands. The changes maintain compatibility with the dual-output logging system introduced in PR #4483.

@cidrblock cidrblock requested a review from a team as a code owner July 28, 2025 14:53
@cidrblock cidrblock changed the title Convert dependency logger from cached instance to property Convert loggers from cached instance to property Jul 28, 2025
@cidrblock cidrblock changed the title Convert loggers from cached instance to property Provide step information in additional log messages Jul 28, 2025
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 addresses a caching issue with loggers in Molecule's core components that prevented them from displaying dynamic scenario and step context during execution. The solution converts static logger instance variables to dynamic properties that retrieve fresh context on each access.

  • Convert logger instance variables to @Property methods across multiple classes
  • Each property now dynamically retrieves current scenario and step context using getattr
  • Enable consistent log message formatting with scenario and step information

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/molecule/dependency/base.py Converts _log from instance variable to property for dynamic context retrieval
src/molecule/dependency/shell.py Removes static logger initialization, inherits dynamic logger from base
src/molecule/dependency/ansible_galaxy/base.py Removes static logger initialization, inherits dynamic logger from base
src/molecule/provisioner/ansible.py Converts _log to property and removes init method
src/molecule/provisioner/ansible_playbook.py Converts _log from instance variable to property
src/molecule/provisioner/ansible_playbooks.py Converts _log from instance variable to property
src/molecule/verifier/ansible.py Converts _log to property and removes init method
src/molecule/verifier/testinfra.py Converts _log from instance variable to property
src/molecule/command/create.py Converts _log to property and removes init method
src/molecule/command/destroy.py Converts _log to property and removes init method
tests/unit/dependency/test_shell.py Updates test to work with dynamic logger context

Command classes will be handled separately with base.Base consolidation.
This PR now focuses only on dependency, provisioner, and verifier classes.
@cidrblock cidrblock merged commit 6ab1ea1 into ansible:main Jul 28, 2025
21 checks passed
cidrblock added a commit that referenced this pull request Jul 28, 2025
…-aware logging (#4488)

## Summary

Comprehensive enhancement of Config class and command base logging with
scenario-aware context, building on the foundation from merged PRs #4483
and #4484.

## User-Facing Changes

**Enhanced logging context across all Config and command operations:**

### Before: Generic logging without context
```
DEBUG    Validating schema molecule.yml
WARNING  Driver 'default' is currently in use but config defines 'podman'
INFO     Performing prerun with role_name_check=0...
```

### After: Complete scenario and step context
```
DEBUG    smoke ➜ validate: Validating schema /path/to/molecule.yml
WARNING  smoke ➜ config: Driver 'default' is currently in use but config defines 'podman'  
INFO     smoke ➜ prerun: Performing prerun with role_name_check=0...
INFO     smoke ➜ discovery: scenario test matrix: syntax
```

## Technical Implementation

**🏗️ Config Class Enhancements:**
- Add reusable `@property _log` method to Config class for dynamic
scenario context
- Convert all Config logging calls to use scenario-aware loggers
- Enhanced validation logging with hardcoded scenario context during
bootstrap
- Improved galaxy file and driver validation warnings with full context

**🧹 Command Base Logger Consolidation:**
- Replace 5 scattered `get_scenario_logger()` calls with single global
`_log()` function
- Unified logging pattern: `_log(scenario_name, step, message,
level='info')`
- Pre-formatted messages eliminate complex placeholder handling
- Cleaner, more maintainable logger creation across all command
operations

**✅ Enhanced Test Coverage:**
- Updated `test_validate` to use `caplog` instead of mock patching
- Validates scenario logger extras: `molecule_scenario` and
`molecule_step`
- Robust verification of message content and context attributes

## Changes Included

**Config Class (4 commits):**
1. `feat: add step context to command base logger calls`
2. `feat: add scenario context to config file modification warning`  
3. `refactor: add reusable _log property to Config class`
4. `refactor: convert all Config logging to scenario-aware logging`

**Command Base Improvements (2 commits):**
5. `refactor: replace scattered logger initializations with global _log
function`
6. `fix: update test_validate to use caplog and check scenario logger
extras`

**Dependencies (Included):**
- Builds on: All cleanup commits from PR #4487 (unused logger removal)

## Dependencies

- **Depends on**: PR #4487 (unused logger cleanup)
- **Builds on**: PR #4484 (merged - dynamic properties for
dependency/provisioner/verifier)
- **Foundation**: PR #4483 (merged - enhanced logging adapter with
dual-output)

## Testing

- ✅ **Comprehensive**: All config tests passing (45/45)
- ✅ **Enhanced coverage**: Validates scenario logger extras in test
suite
- ✅ **Functional verification**: `molecule syntax` confirms enhanced
logging works
- ✅ **Backwards compatible**: No breaking changes to existing
functionality

## Impact

- **Complete Config context**: All Config operations now include
scenario and step information
- **Unified command logging**: Consistent pattern across all command
base operations
- **Better maintainability**: Single global `_log` function replaces
scattered initializations
- **Enhanced debugging**: Full context for configuration validation and
driver operations

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

2 participants