feat(task): Add support for passing down cwd to spawn steps #3209
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3209 +/- ##
=======================================
Coverage 96.32% 96.32%
=======================================
Files 191 191
Lines 36488 36498 +10
Branches 3399 3399
=======================================
+ Hits 35147 35158 +11
+ Misses 1341 1340 -1 ☔ View full report in Codecov by Sentry. |
icj217
force-pushed
the
cwd_spawn_support
branch
from
af7c8f6
to
78ebd35
Compare
icj217
force-pushed
the
cwd_spawn_support
branch
from
78ebd35
to
89396ec
Compare
test/tasks/runtime.test.ts
Outdated
| test("cwd can be passed down to spawn steps", () => { | ||
| const p = new TestProject(); | ||
| const taskcwd = join(p.outdir, "mypwd"); | ||
| const stepcwd = join(p.outdir, "yourpwd"); | ||
| mkdirSync(taskcwd, { recursive: true }); | ||
| mkdirSync(stepcwd, { recursive: true }); | ||
| const echo = p.addTask("echo", { exec: "echo", receiveArgs: true }); | ||
| const task = p.addTask("testme", { cwd: taskcwd }); | ||
| task.spawn(echo, { args: ["step1=$PWD"] }); | ||
| task.spawn(echo, { args: ["step1=$PWD"], cwd: stepcwd }); | ||
|
|
||
| const noCwdSpecified = executeTask(p, "echo", {}, ["$PWD"]); | ||
| expect(noCwdSpecified[0]).toContain(p.outdir); | ||
|
|
||
| const lines = executeTask(p, "testme"); | ||
| expect(lines[0].includes("mypwd")).toBeTruthy(); | ||
| expect(lines[1].includes("yourpwd")).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
I've tried hard to understand what this test is doing. Could you please refactor this to be more readable. I'd recommend sticking to the same names for things e.g. not taskcwd and mypwd and make the symbol names more explicit.
There was a problem hiding this comment.
Good point. I've updated the test to hopefully be clearer now.
test/tasks/runtime.test.ts
Outdated
| const noCwdSpecified = executeTask(p, "echo", {}, ["$PWD"]); | ||
| expect(noCwdSpecified[0]).toContain(p.outdir); |
There was a problem hiding this comment.
This seems redundant since very task on this project will use the project's outdir.
| @@ -153,7 +162,8 @@ class RunTask { | |||
| this.runtime.runTask( | |||
| step.spawn, | |||
| [...this.parents, this.task.name], | |||
| argsList | |||
| argsList, | |||
| { cwd: step.cwd ?? task.cwd } | |||
There was a problem hiding this comment.
This looks like a breaking change to me. We are now giving the step.cwd preference over the task.cwd and there is no way to revert the original behavior?!
There was a problem hiding this comment.
Yeah my struggle with implementing this is that it doesn't seem like the step-level cwd was originally intended to be used outside of exec steps, but none of the documentation indicates this, so it seems logical that if you set an explicit step-level cwd, then it should take precedence regardless of whether that step is an exec or a spawn. step.cwd as well as task.cwd can both be undefined, which would result in the default runtime working dir taking precedence (as seen on line 101)
We could add a new option to TaskSpec like spawnInheritCwd?: boolean to make this behavior explicit, but aesthetically that doesn't feel great.
|
There will be bigger changes and some deprecations as part of #3162 |
The existing documentation around the
cwdtask/step property doesn't explicitly state that it only works forexecsteps, but that's currently the case. There are use cases where havingspawntasks inherit the cwd of the task that spawned them is quite useful. For example:By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.