-
Notifications
You must be signed in to change notification settings - Fork 4
NEW @W-19397402@ Implemented hidden config property for keeping working d… #369
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
| log_level: LogLevel | ||
| rules: Record<string, RuleOverrides> | ||
| engines: Record<string, EngineOverrides> | ||
| preserve_all_working_directories: boolean // INTERNAL USE ONLY |
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.
For properties that are potentially customer facing, let's be consistent. We are calling it "working folders" and not directories. So I propose we call this something like "preserve_working_folders_always" instead.
| * Returns a boolean indicating whether working directories should be preserved even if their engine succeeds, as opposed | ||
| * to being kept only for failed engines. |
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.
May I suggest:
* Returns a boolean indicating whether working folders are preserved always.
* If false, then the working folders for an engine are deleted unless the engine issues an error.
* If true, then the working folders for each engine remain, even if an engine does not issues any error.
| if (this.config.getPreserveAllWorkingDirectories()) { | ||
| this.tempFolder.markToBeKept(runWorkingFolderName); | ||
| this.emitLogEvent(LogLevel.Debug, getMessage('AllWorkingFoldersKept', await this.tempFolder.getPath(runWorkingFolderName))); | ||
| } |
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 this.tempFolder.markToBeKept(runWorkingFolderName); isn't needed. Line 330 already marks the subfolders as kept and therefore the parent folder (runWorkingFolderName) is already kept by the way the temp folder preserves parents.
I'm on the fence about needing a single debug message. We could just debug each engine folder instead to keep a log code simple with the log being below line 330. But I see the motivation here - to have a single log for all things under the run folder.
| }); | ||
|
|
||
| if (this.config.getPreserveAllWorkingDirectories()) { | ||
| this.tempFolder.markToBeKept(rulesWorkingFolderName); |
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.
ditto - not needed.
…irectories