-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add pre and post command support #666
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
{ | ||
if (usesTempFile) | ||
{ | ||
File.Delete(fileName); |
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.
Maybe try/catch but just log if there is an error and not break the run.
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.
Have added one, let me know if it is behaving the way you were thinking
{ | ||
public enum ShellType | ||
{ | ||
Batch, |
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.
Isn't it Cmd
(aka command prompt). Batch is the name of the script files.
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've renamed this to ScriptType
since that is a more accurate description than ShellType
, this defines what type of script it is
src/Microsoft.Crank.Models/Job.cs
Outdated
@@ -281,6 +281,14 @@ public Source Source | |||
/// </summary> | |||
public bool PatchReferences { get; set; } = false; | |||
|
|||
public Dictionary<string, List<CommandDefinition>> Commands { get; set; } | |||
|
|||
public List<string> PreCommandOrder { get; set; } = new(); |
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 don't like "Pre" and I don't like "Order". Let's assume the array/list is ordered, so not mention that.
In msbuild then have BeforeTarget
/AfterTarget
, so here I would suggest BeforeJob
/AfterJob
and assume these are command names without repeating the work command.
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 am wondering if these should have arguments. After all the scripts could read arguments. These could be a list of name/value properties on each command. So the string[] becomes a Command[] with { name, arguments[] { name, value }
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've made the name change to BeforeJob
and AfterJob
. I was thinking about how it would look to add arguments to the commands, but since the commands support variable substitution, I think that variable substitution feels more natural as the approach to parameterisation here. I know at least in my use case for this feature I plan on using > 15 variables and it'd be tedious to have to define all those arguments I think.
We'll need some doc/sample to remember how it works. |
test/Microsoft.Crank.IntegrationTests/assets/precommands.benchmarks.yml
Outdated
Show resolved
Hide resolved
Co-authored-by: Sébastien Ros <sebastienros@gmail.com>
Summary
This PR adds a feature to crank that allows you to define commands that should be run on the controller machine before and after the run. With this feature, it now becomes possible to write commands to pre-organise or pre-build things on the controller machine before the run starts.
Motivation
The motivation for this was the work towards being able to run benchmarks using crank from the dotnet/performance repository against a local build of the dotnet runtime. Since the runtime is very large, and takes a long time to build, it is better if instead the dotnet runtime is built locally first, and then the build output folder is uploaded to the crank agent and run against that instead. This means that every time before the crank command is run, the developer will need to re-build the runtime each time. With this functionality, it means that we can add pre-commands to crank that will trigger a rebuild of the runtime using all the correct build arguments needed for our scenarios.
Another use case might be that the files that you want to be uploaded to crank may all live in different folders and you don't want to upload all those folders if they contain irrelevant files in them. With this, you could write a pre-command that copies all the files into a temporary directory so that it can be uploaded to the crank agent, then a post-command could then delete that folder afterwards if preferred.
Implementation
An example of a yaml file with commands is below:
Commands are defined under
commands
either at the root level or at the job level. The command definitions are merged together with the job-level definitions taking precedence over the root-level definitions. Each command has a name, and multiple definitions where each definition has a condition in the form of a javascript expression. Global properties such asjob
andconfiguration
exist to use in the javascript expression to be able to change the command that is run. If multiple command definitions are true for a given run, the first one that appears in the list will be the one invoked. Also as shown in the sample, it is possible to make use of the job variables inside a command definition.These are all the properties available inside a command definition:
The PR also adds a new field under
Job
calledEnvironment
which has two fields:Platform
which has a value ofwindows | linux | osx | other
andArchitecture
which has valuesX86 | X64 | ARM | ARM64
. These are useful to use from the conditions which will most likely need to be different for each platform.Some other design decisions
ProcessUtil
out of the Pull Request Bot into the Controller project since it was useful to implement this feature. The Pull Request Bot already has a reference to the Controller project, but there wasn't really any better place to put common code to be reused among these projects.I've verified all this functionality locally on my machine as well and it works quite well, but I'm open to changing the design here as well if needed.