Use Task Executor Workdir #281
Conversation
|
/azp run |
src/TesApi.Web/BatchScheduler.cs
Outdated
| @@ -1106,7 +1106,7 @@ private async Task<CloudTask> ConvertTesTaskToBatchTaskAsync(string taskId, TesT | |||
| sb.AppendLinuxLine($"chmod -R o+rwx $AZ_BATCH_TASK_WORKING_DIR/wd && \\"); | |||
| sb.AppendLinuxLine($"export TES_TASK_WD=$AZ_BATCH_TASK_WORKING_DIR/wd && \\"); | |||
| sb.AppendLinuxLine($"write_ts ExecutorStart && \\"); | |||
| sb.AppendLinuxLine($"docker run --rm {volumeMountsOption} --entrypoint= --workdir / {executor.Image} {executor.Command[0]} \"{string.Join(" && ", executor.Command.Skip(1))}\" && \\"); | |||
| sb.AppendLinuxLine($"docker run --rm {volumeMountsOption} --entrypoint= --workdir {executor.Workdir ?? "/"} {executor.Image} {executor.Command[0]} \"{string.Join(" && ", executor.Command.Skip(1))}\" && \\"); | |||
There was a problem hiding this comment.
Suggestion: use string.IsNullOrWhitespace
Even better suggestion: remove --workdir entirely if/when there's no non-whitespace text in executor.Workdir, which would bring this implementation of TES into conformity with the TES specification.
There was a problem hiding this comment.
It would change the default behavior right? running on the container workdir vs on root, are we good with that?
There was a problem hiding this comment.
I will talk with the team on Friday about this, but the TES spec is clear that if this parameter is not set then the container default is to be used.
There was a problem hiding this comment.
Its fine by me, we are always passing a workdir, and I would be happy to push that change. I just wanted to make sure that there are no regressions or breaking changes for current users
|
/azp run |
|
the creds secret aren't used in the dotnet-format action, maybe because its from a fork? |
That's definitely possible. Our azure CI/CD also doesn't work with forks (I've had to copy this branch to this repo in order to build your PR to run our integration tests, etc.) |
|
In that case, can I get permission to create a branch in the repo? Or can this be merged without the check? |
We can merge it without the check. I expect to complete validation by mid-week. |
|
@SharonHart Will the website allow you to merge? |
Nope :( |
Addresses #282