Unified logger pass 3 #2701
Unified logger pass 3 #2701
Conversation
…and in one file with consistent formatting.
- Add dynamic dev‑env log filenames (slug + timestamp) and wire bootstrapLando({ logFile }) across all dev‑env commands.
- Update lando dependency in vip‑cli to new fork commit.
Dependency ReviewThe following issues were found:
License Issuespackage.json
OpenSSF Scorecard
Scanned Files
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
There was a problem hiding this comment.
Pull request overview
This PR implements unified logging for VIP Dev Env commands by integrating command execution logs with Lando. Each dev-env command now generates a timestamped log file that combines output from both Lando and VIP-CLI, stored in $HOME/.local/share/vip/lando/logs/. The log path is displayed at the end of each command execution.
Changes:
- Added TypeScript type definitions for Lando logger enhancements including
logFile,logName, andchild()method - Implemented log file path generation with timestamp and slug formatting in dev-environment-cli.ts
- Enhanced Lando bootstrap process with log banner generation, Docker version detection, and system information logging
- Updated all 13 dev-env command files to pass log file configuration to
bootstrapLando() - Updated lando dependency to commit
8f198e0f17dec1144efa216bec5a227dfc851900 - Updated npm-shrinkwrap.json with new dependencies (@isaacs/balanced-match, @isaacs/brace-expansion) and corrected peer/dev flags
Reviewed changes
Copilot reviewed 4 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| types/lando/lib/logger.d.ts | Added type definitions for new logger options (logFile, logName, logger) and child() method |
| src/lib/dev-environment/dev-environment-lando.ts | Implemented log file path handling, banner generation with system info, Docker version detection, and debug log redirection |
| src/lib/dev-environment/dev-environment-cli.ts | Added utility functions for log file naming: formatDevEnvLogTimestamp, formatDevEnvLogSlug, and getDevEnvLogFile |
| src/commands/dev-env-import-sql.ts | Updated to pass slug-specific log file to bootstrapLando |
| src/bin/vip-dev-env-*.js (13 files) | Updated all dev-env commands to call bootstrapLando with getDevEnvLogFile for consistent logging |
| package.json | Updated lando dependency to new commit hash |
| npm-shrinkwrap.json | Updated dependency tree with new modules, corrected peer/dev flags, removed figlet/through/valid-url dependencies |
Files not reviewed (1)
- npm-shrinkwrap.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function formatDevEnvLogTimestamp( date: Date ): string { | ||
| return date.toISOString().replace( /\..*$/, '' ).replace( /[-:]/g, '' ).replace( 'T', '-' ); | ||
| } | ||
|
|
||
| export function formatDevEnvLogSlug( slug: string ): string { | ||
| return slug.replace( /[^a-z0-9_-]+/gi, '-' ).toLowerCase(); | ||
| } | ||
|
|
||
| export function getDevEnvLogFile( slug?: string ): string { | ||
| const slugPart = slug ? formatDevEnvLogSlug( slug ) : 'all'; | ||
| return `vip-dev-env-${ slugPart }-${ formatDevEnvLogTimestamp( new Date() ) }.log`; | ||
| } |
There was a problem hiding this comment.
The three new exported functions formatDevEnvLogTimestamp, formatDevEnvLogSlug, and getDevEnvLogFile lack test coverage. These functions handle string formatting and timestamp generation, which are important for log file naming. Consider adding unit tests to verify the expected output format, especially for formatDevEnvLogTimestamp which manipulates dates, and formatDevEnvLogSlug which sanitizes slug strings.
| try { | ||
| const existing = await stat( logFilePath ); | ||
| if ( existing.size > 0 ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The writeLogBanner function writes to the log file only if it doesn't exist or has size 0. However, if the file exists and is empty due to a previous failed write, this check will still skip writing the banner. Consider also checking for file age or using a more robust mechanism to determine if the banner should be written, such as checking if the banner is already present in the file.
| await mkdir( path.dirname( logFilePath ), { recursive: true } ); | ||
|
|
||
| const dockerVersions = await getDockerVersions( config ); | ||
| const command = process.argv.slice( 1 ).join( ' ' ); |
There was a problem hiding this comment.
The function uses process.argv.slice(1).join(' ') to construct the command line, which may not properly represent arguments that contain spaces or special characters. Consider using proper shell escaping or quoting for arguments that contain spaces to ensure the logged command accurately reflects what was executed.
| const command = process.argv.slice( 1 ).join( ' ' ); | |
| const command = process.argv.slice( 1 ).map( ( arg ) => JSON.stringify( arg ) ).join( ' ' ); |
| if ( process.env.DEBUG && ! process.env.DEBUG.includes( '-winston:*' ) ) { | ||
| process.env.DEBUG = `${ process.env.DEBUG },-winston:*`; |
There was a problem hiding this comment.
The DEBUG environment variable modification could interfere with user's existing debug settings. Adding -winston:* to an existing DEBUG value without checking if it's already present could result in duplicate filters. Consider checking if the filter is already present before appending it, or use a more robust approach to exclude winston logs.
| if ( process.env.DEBUG && ! process.env.DEBUG.includes( '-winston:*' ) ) { | |
| process.env.DEBUG = `${ process.env.DEBUG },-winston:*`; | |
| if ( process.env.DEBUG !== undefined ) { | |
| const debugEnv = process.env.DEBUG; | |
| const patterns = debugEnv | |
| .split( ',' ) | |
| .map( ( pattern ) => pattern.trim() ) | |
| .filter( ( pattern ) => pattern.length > 0 ); | |
| if ( ! patterns.includes( '-winston:*' ) ) { | |
| patterns.push( '-winston:*' ); | |
| process.env.DEBUG = patterns.join( ',' ); | |
| } |
| debugLib.log = ( message: string, ...args: unknown[] ) => { | ||
| lando.log.debug( message, ...args ); |
There was a problem hiding this comment.
Overriding the global debugLib.log function affects all debug instances in the entire application, not just those related to this module. This side effect could interfere with debugging in other parts of the codebase. Consider using a more targeted approach, such as creating a child logger or wrapping the debug instance locally.
| debugLib.log = ( message: string, ...args: unknown[] ) => { | |
| lando.log.debug( message, ...args ); | |
| ( debug as debugLib.Debugger ).log = ( ...args: unknown[] ) => { | |
| lando.log.debug( ...args ); |
Description
This PR improves logging integration with Lando, now each dev-env command run generates a single log file that combines run results both from Lando and VIP-CLI/Dev Env. At the end of command run we now show where this file is:
Needs Automattic/lando-cli#170
============
This pull request updates the
npm-shrinkwrap.jsonfile with several dependency and metadata adjustments. The most notable changes include updating the version of thelandodependency, adding new modules, and correcting or addingpeeranddevflags for various dependencies to improve package resolution and compatibilityDependency updates and additions:
landodependency to a new commit hash forgithub:automattic/lando-cli, ensuring the project uses the latest version from the repository.@isaacs/balanced-matchand@isaacs/brace-expansion, including their versioning, integrity, and engine requirements.Peer and dev flag corrections:
"peer": trueflag for multiple dependencies, such as@babel/core,acorn,deep-is,eslint,ajv, and others, to accurately represent peer dependency relationships and prevent npm/yarn warnings. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22] [23] [24] [25]"dev": trueto several dependencies to clarify their development-only status, such as@isaacs/cliui,ansi-regex,ansi-styles,string-width,strip-ansi,wrap-ansi,@pkgjs/parseargs,eastasianwidth, andemoji-regex. [1] [2] [3] [4] [5] [6] [7] [8]These changes help ensure that the dependency tree is accurate, up-to-date, and that package managers handle peer and dev dependencies correctly, reducing potential install-time issues and improving maintainability.
Changelog Description
Added
Pull request checklist
New release checklist
Steps to Test
Outline the steps to test and verify the PR here.
Example:
npm run build./dist/bin/vip-cookies.js nom