-
Notifications
You must be signed in to change notification settings - Fork 103
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
Refactor log file handling to standardize zip creation and improve er… #1950
Conversation
ee/debug/checkups/power_windows.go
Outdated
powerCfgSleepStatesCmd, err := allowedcmd.Powercfg(ctx, "/availablesleepstates") | ||
if err != nil { | ||
return fmt.Errorf("creating powercfg sleep states command: %w", err) | ||
} | ||
|
||
hideWindow(powerCfgSleepStatesCmd) | ||
availableSleepStatesOutput, err := powerCfgSleepStatesCmd.CombinedOutput() | ||
sleepStatesOutput, err := powerCfgSleepStatesCmd.Output() |
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.
what's the reason for this change? I'm not sure we'd want to lose the combined output here, I don't think err alone is typically very helpful for debugging with these
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.
That's fair, I oversimplified here. Adding the combined output back
} | ||
|
||
return nil | ||
return addFileToZip(z, "/var/log/install.log") |
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.
I think this has the inadvertent effect that it's going to cause the end files to be named something else, is that correct? I don't think that's inherently wrong, but I want to note it.
I think we might even want it, since I'd thought about the idea of moving all the files into a dedicated file directory, instead of mini zip files for each flare.
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.
That's correct the file will always be install.log instead of varying for macos
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.
Nice work!
This pull request introduces significant changes to the
checkups
package, focusing on improving the handling of file zipping operations. The changes include adding new helper functions for zipping files and streams, and refactoring existing code to utilize these new functions.Resolves #1949
Improvements to file zipping operations:
ee/debug/checkups/util.go
: Added new helper functionaddStreamToZip
to streamline the process of adding files and streams to a zip archive with metadata. [1] [2] [3] [4]Refactoring existing code to use new helper functions:
ee/debug/checkups/download_directory.go
: Updated theRun
method to useaddFileToZip
for adding files to the zip archive and changed the extra file name tokolide-installers-all-users.zip
. [1] [2] [3] [4]ee/debug/checkups/init_logs_darwin.go
,ee/debug/checkups/init_logs_linux.go
,ee/debug/checkups/init_logs_windows.go
: Refactored thewriteInitLogs
function to useaddFileToZip
andaddStreamToZip
for zipping log files and command outputs. [1] [2] [3] [4] [5] [6]ee/debug/checkups/install.go
: Simplified thegatherInstallationLogs
andgatherInstallerInfo
functions by usingaddFileToZip
for zipping installation logs and installer info files. [1] [2]ee/debug/checkups/launchd_darwin.go
: Updated theRun
method to useaddFileToZip
andaddStreamToZip
for zipping plist files and command outputs. [1] [2] [3]ee/debug/checkups/logs.go
: Refactored theRun
method to useaddFileToZip
for adding log files to the zip archive.ee/debug/checkups/power_windows.go
: Simplified theRun
method by usingaddFileToZip
andaddStreamToZip
for zipping power reports and command outputs. [1] [2]…ror reporting