-
Notifications
You must be signed in to change notification settings - Fork 30
Add more logging to CLI invocations on CI and ensure CLI invocations have timeouts on CI #1964
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
Conversation
…have timeouts on CI
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 adds comprehensive logging and timeout handling to CLI invocations in the documentation system, particularly for CI environments. The changes are designed to improve observability and prevent hanging processes during automated builds.
Key changes:
- Added timeout configuration for external command execution in CI environments (10 minutes)
- Enhanced logging throughout CLI operations with structured logging patterns
- Refactored command execution methods to consolidate duplicate logic and improve error handling
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
ExternalCommandExecutor.cs |
Core refactoring of command execution with timeout support, enhanced logging, and consolidated capture methods |
GitFacade.cs |
Added logging factory and CI timeout configuration to git operations |
RepositorySourcesFetcher.cs |
Updated git repository instantiation to include logging factory parameter |
AwsCloudFrontKeyValueStoreProxy.cs |
Migrated from private logger to inherited Logger property and updated method signature |
LocalGitRepositoryTracker.cs |
Added logging factory support and updated to use CaptureQuiet method |
LocalChangesService.cs |
Updated constructor call to include logging factory parameter |
Comments suppressed due to low confidence (1)
src/Elastic.Documentation/ExternalCommands/ExternalCommandExecutor.cs:1
- The method signature changed from
CaptureMultiple(false, 1, ...)
toCaptureMultiple(1, ...)
, but the new overload defaults tomuteExceptions = false
. This maintains the same behavior, but the change appears to remove the explicitmuteExceptions
parameter that was previously set tofalse
.
// Licensed to Elasticsearch B.V under one or more agreements.
var arguments = new ExecArguments(binary, args) | ||
{ | ||
WorkingDirectory = workingDirectory.FullName, | ||
Environment = environmentVars | ||
Environment = environmentVars, | ||
Timeout = timeout | ||
}; |
Copilot
AI
Sep 30, 2025
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.
The timeout is only applied when environmentVars is provided, but not in the regular Exec methods without environment variables. This creates inconsistent timeout behavior across different execution paths.
Copilot uses AI. Check for mistakes.
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.
This is false timeout is always provided for ExecIn*
var arguments = new StartArguments(binary, args) | ||
{ | ||
WorkingDirectory = workingDirectory.FullName, | ||
Timeout = TimeSpan.FromSeconds(3), | ||
WaitForExit = TimeSpan.FromSeconds(3), |
Copilot
AI
Sep 30, 2025
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.
The hardcoded 3-second timeout in CaptureOutput conflicts with the configurable timeout parameter. Consider using the configurable timeout or a derived value to maintain consistency.
var arguments = new StartArguments(binary, args) | |
{ | |
WorkingDirectory = workingDirectory.FullName, | |
Timeout = TimeSpan.FromSeconds(3), | |
WaitForExit = TimeSpan.FromSeconds(3), | |
var effectiveTimeout = timeout ?? TimeSpan.FromSeconds(3); | |
var arguments = new StartArguments(binary, args) | |
{ | |
WorkingDirectory = workingDirectory.FullName, | |
Timeout = effectiveTimeout, | |
WaitForExit = effectiveTimeout, |
Copilot uses AI. Check for mistakes.
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.
This is by design, capturing is retried and is expected to run quickly.
No description provided.