Repository navigation
Support post build rush hooks - #196
Conversation
QZ (@qz2017) looks like you need to join your GitHub account to the Microsoft organization. #Resolved |
| public onPostCommand(): void { | ||
| this._postCommandHooks.forEach((script) => { | ||
| Utilities.executeShellCommand(script); | ||
| }); |
There was a problem hiding this comment.
FYI I've been trying to avoid putting business logic in the configuration classes.
RushConfiguration's job is to read (and sometimes write) config files for Rush. By itself, that is already a complicated problem with lots of code. #Resolved
|
|
||
| } | ||
|
|
||
| // (undocumented) |
There was a problem hiding this comment.
Add complete documentation: what is this for, how is it used, etc. #Resolved
There was a problem hiding this comment.
In general, don't introduce any new definitions to api.ts that are missing documentation
In reply to: 116885084 [](ancestors = 116885084)
There was a problem hiding this comment.
|
|
||
| // (undocumented) | ||
| class RushHooks { | ||
| // WARNING: The type "IRushHooksJson" needs to be exported by the package (e.g. added to index.ts) |
There was a problem hiding this comment.
Fix this warning #Resolved
| return this._postCommandHooks; | ||
| } | ||
|
|
||
| public onPostCommand(): void { |
There was a problem hiding this comment.
onPostCommand [](start = 9, length = 13)
The "on" prefix is used for event hooks that are implemented by a separate party.
Compare with a design like this:
execute(hookName: string): void #Resolved
| }); | ||
| } | ||
|
|
||
| public static executeShellCommand( |
There was a problem hiding this comment.
executeShellCommand [](start = 16, length = 19)
We already have executeCommand(), executeCommandAndCaptureOutput(), and executeCommandAsync(). Why do we need ANOTHER function for executing commands?
FYI the NodeJS spawn() sucks, and I have spent a lot of time fixing bugs related to these functions. The handling of escapes is still not 100% correct. My plan is to write a bunch of unit tests that capture all the difficult scenarios, and then migrate this code into a node-core-library API something like this:
class Process {
public execute(command, args, options): void;
public executeAsync(command, args, options): void;
}
If you want to help push that agenda along, let me know. :-) #Resolved
There was a problem hiding this comment.
Because we need a version that does not need parsing arguments.
In reply to: 116887866 [](ancestors = 116887866,116886431)
| workingDirectory?: string, | ||
| environmentVariables?: { [key: string]: string } | ||
| ): void { | ||
| const child: child_process.ChildProcess = child_process.exec( |
There was a problem hiding this comment.
exec [](start = 60, length = 4)
This function is incapable of correctly escaping parameters. Don't use it. #Resolved
| it('loads when there is no rush hooks in rush.json', () => { | ||
| const rushFilename: string = path.resolve(__dirname, 'repo1', 'rush.json'); | ||
| const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(rushFilename); | ||
| expect(rushConfiguration.rushHooks).to.be.undefined; |
There was a problem hiding this comment.
.to.be.undefined [](start = 39, length = 16)
Instead of expect(), we should always call assert(), because it uses conventional TypeScript code, versus inventing a cute nonstandard syntax with limited capabilities.
nickpape-msft
Ian Clanton-Thuon (@iclanton) #Resolved
There was a problem hiding this comment.
I do not think expect is just cute nonstandard syntax. I changed to assert just to be consistent with the rest of test code.
In reply to: 116886940 [](ancestors = 116886940)
| "type": "object", | ||
| "properties": { | ||
| "postCommand": { | ||
| "description": "The list of scripts to run after every Rush command.", |
There was a problem hiding this comment.
"The list of scripts to run after every Rush command." [](start = 25, length = 54)
Which shell do these commands run under? i.e. how do we make them behave consistently on different operating systems?
Might be useful to look at how the NPM tool invokes its scripts. #Resolved
…h other configuration classes, and other minor code refactoring.
|
The config setting we discussed: "telemetryEnabled": "true",
"hooks": {
"postCommand": "node ./common/scripts/upload-telemetry.js"
}
``` #Resolved |
QZ (qz2017)
changed the title
| } | ||
|
|
||
| /** | ||
| * This class represents rush hooks configured for this repo. |
There was a problem hiding this comment.
rush [](start = 25, length = 4)
capitalize Rush #Resolved
There was a problem hiding this comment.
Also maybe give a little more detail:
"Hooks are user-defined script actions that Rush executes when specific events occur. The actions are expressed as a command-line that is executed using the operating system shell."
In reply to: 117120289 [](ancestors = 117120289)
|
|
||
| public handle(hookName: RushHookName): void { | ||
| this._rushHooks.get(hookName).forEach((script) => { | ||
| Utilities.executeCommandOnShell(script, |
There was a problem hiding this comment.
tilities.executeCommandOnShell(script, [](start = 7, length = 38)
Before we execute each command, we should print something to the log window:
Executing "postCommand" hook: rm -Rf some/path
#Resolved
There was a problem hiding this comment.
It is up to individual script to decide whether it wants to print out something.
In reply to: 117121270 [](ancestors = 117121270)
There was a problem hiding this comment.
I'm concerned that if a poorly written script is printing errors, people will report problems to the Rush maintainers, because it looks like Rush is failing.
In reply to: 117124061 [](ancestors = 117124061,117121270)
| * @param workingDirectory - working directory for running this command | ||
| * @param environmentVariables - environment variables for running this command | ||
| * @alpha | ||
| */ |
There was a problem hiding this comment.
executeCommandOnShell [](start = 16, length = 21)
"on shell" is unusual terminology. I suggest executeShellCommand()
#Resolved
| "sampleEmail": "mrexample@contoso.com" | ||
| }, | ||
|
|
||
| "rushHooks": { |
There was a problem hiding this comment.
rushHooks [](start = 3, length = 9)
Why is it "rushHooks"? We're already in the rush.json config file.
(I am sorry to hassle you about this, but you are using a telemetry PR to introduce a general event handling mechanism into Rush without a spec or design discussion. So it's inevitable that the code review will be a little bumpy.)
Thinking about the name... maybe the feature should be called "eventHooks" (or "eventScripts"?), and then the "postBuild" event should be "afterRushBuild". That would allow us to introduce non-Rush-specific hooks later, e.g. "afterNpmInstall".
Ian Clanton-Thuon (@iclanton) nickpape-msft #Resolved
|
|
3 participants
A new section in RushConfiguration to specify rush hook.
Support hooks for post rush build command