SPLIT: Original PR split into focused PRs #4485a and #4485b#4485
Closed
cidrblock wants to merge 11 commits intoansible:mainfrom
Closed
SPLIT: Original PR split into focused PRs #4485a and #4485b#4485cidrblock wants to merge 11 commits intoansible:mainfrom
cidrblock wants to merge 11 commits intoansible:mainfrom
Conversation
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)
8ed3ec4 to
35ba18c
Compare
Remove test file changes that belong in other PRs: - test_shell.py: Scenario context testing belongs in dependency logger fix PR - test_ansi_output.py: Environment detection testing was deferred Keep this PR focused only on removing unused logger definitions.
4b7005b to
1d8c1d2
Compare
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
Author
|
Closing this PR in favor of clean, focused PRs with proper dependency ordering against upstream main. Replaced by:
This approach provides cleaner commit history and proper review dependency chain. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔄 This PR has been split for better review
This original PR contained mixed concerns and has been split into two focused PRs:
📦 PR #4485a: Pure Cleanup
Branch:
feat/cleanup-unused-loggers-onlyScope: Remove unused
LOG = logging.getLogger(__name__)definitionsStatus: Ready for review
URL: cidrblock/molecule@main...feat/cleanup-unused-loggers-only
🏗️ PR #4485b: Dynamic Logger Architecture
Branch:
feat/dynamic-logger-propertiesScope: Convert cached loggers to dynamic
@propertypattern + Config enhancementsStatus: Ready for review
URL: cidrblock/molecule@main...feat/dynamic-logger-properties
🎯 Split Benefits
📋 Review Strategy
🔧 Technical Split Details
Cleanup-only commits (→ #4485a):
cleanup: remove unused logger definitions from logger.py, syntax.py, and state.pycleanup: remove unused logger definitions from 18+ filescleanup: remove final unused LOG definition from base.pyArchitectural commits (→ #4485b):
refactor: convert _log to property in dependency, provisioner, verifier and command classesstyle: remove redundant __init__ methodsfeat: add step context to command base logger callsfeat: add scenario context to config file modification warningrefactor: add reusable _log property to Config classrefactor: convert all Config logging to scenario-aware loggingThis PR will be closed once the split PRs are merged.