Store TaskDefinitions in the Graph by Task ID #3428
Conversation
This means we look up a task's dependencies when iterating per-workspace, rather than per-task. This will allow us to customize each task per workspace based on additional config that may be present in that workspace
This will enable us to compose TaskDefinitions per workspace, so they can be distinctly looked up.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Ignored Deployments
|
| if !notcool { | ||
| // Return an empty fs.TaskDefinition | ||
| return nil, fmt.Errorf("No task defined in pipeline") | ||
| taskDefinition, ok := g.TaskDefinitions[taskID] |
There was a problem hiding this comment.
Instead of looking for a taskDefinition in the Pipeline, we now look for it in the TaskDefinitions map. The TaskDefinitions in this map will later become composed from multiple configs.
| @@ -388,7 +389,7 @@ func (r *run) initCache(ctx gocontext.Context, rs *runSpec, analyticsClient anal | |||
| } | |||
|
|
|||
| func buildTaskGraphEngine(g *graph.CompleteGraph, rs *runSpec) (*core.Engine, error) { | |||
| engine := core.NewEngine(&g.WorkspaceGraph) | |||
| engine := core.NewEngine(g) | |||
There was a problem hiding this comment.
This is not great, and it really blurs the line between Engine and Graph. This line already seems pretty blurry to me, so I made the call to pass the whole thing in, rather than just TaskDefinitions, Pipeline, etc. There's some room for improvement with this architecture after Composable Turbo is done.
| "libB": &fs.PackageJSON{}, | ||
| "libC": &fs.PackageJSON{}, | ||
| "libD": &fs.PackageJSON{}, | ||
| }, |
There was a problem hiding this comment.
All these engine tests are really coupled to the implementation, so they have to change when things change. I would like to convert a lot of them to integration tests with prysk, so we can validate what we really need to validate.
| @@ -328,6 +363,7 @@ func (e *Engine) ValidatePersistentDependencies(graph *graph.CompleteGraph) erro | |||
| packageName, taskName := util.GetPackageTaskFromId(depTaskID) | |||
|
|
|||
| // Get the Task Definition so we can check if it is Persistent | |||
| // TODO(mehulkar): Do we need to get a resolved taskDefinition here? | |||
There was a problem hiding this comment.
I have a separate ticket for this
| @@ -174,6 +174,8 @@ func (th *Tracker) CalculateFileHashes(allTasks []dag.Vertex, workerCount int, r | |||
| continue | |||
| } | |||
|
|
|||
| // TODO(mehulkar): Once we start composing turbo.json, we need to change this | |||
| // to look in the graph for TaskDefinitions, rather than the root pipeline. | |||
There was a problem hiding this comment.
I have another ticket for this. Things should still work correctly after this PR, since there is only one turbo.json so far.
Co-authored-by: Chris Olszewski <chris.olszewski@vercel.com>
🟢 CI successful 🟢Thanks |
since we don't need the pkg to look up turbo.jsons in this refactor we don't need this other code yet
Moves TaskDefinitions in the
CompleteGraphstruct, so we can look it up during execution.We want to do this here, because during execution, The TaskGraph has already been constructed,
so it is too late to change the definition at the point. Although this commit does not try to
change any TaskDefinitions, we will implement that for composable configs in the future.