Skip to content
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

DEVPROD-11189 Resolve execution and display tasks correctly for API Patches #8389

Merged
merged 6 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions model/project.go
Original file line number Diff line number Diff line change
Expand Up @@ -1683,6 +1683,28 @@ func (p *Project) IgnoresAllFiles(files []string) bool {
// unmatched requester (e.g. a patch-only task for a mainline commit).
func (p *Project) BuildProjectTVPairs(patchDoc *patch.Patch, alias string) {
patchDoc.BuildVariants, patchDoc.Tasks, patchDoc.VariantsTasks = p.ResolvePatchVTs(patchDoc, patchDoc.GetRequester(), alias, true)

// Connect the execution tasks to the display tasks.
displayTasksToExecTasks := map[string][]string{}
for _, bv := range p.BuildVariants {
for _, dt := range bv.DisplayTasks {
displayTasksToExecTasks[dt.Name] = dt.ExecTasks
}
}

vts := []patch.VariantTasks{}
for _, vt := range patchDoc.VariantsTasks {
dts := []patch.DisplayTask{}
for _, dt := range vt.DisplayTasks {
if ets, ok := displayTasksToExecTasks[dt.Name]; ok {
dt.ExecTasks = ets
}
dts = append(dts, dt)
}
vt.DisplayTasks = dts
vts = append(vts, vt)
}
patchDoc.VariantsTasks = vts
}

// ResolvePatchVTs resolves a list of build variants and tasks into a list of
Expand Down
5 changes: 4 additions & 1 deletion model/project_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1371,7 +1371,10 @@ func (s *projectSuite) TestBuildProjectTVPairsWithDisplayTaskWithDependencies()
s.Contains(vt.Tasks, "very_task")
s.Require().Len(vt.DisplayTasks, 1)
s.Equal("memes", vt.DisplayTasks[0].Name)
s.Empty(vt.DisplayTasks[0].ExecTasks)
s.Len(vt.DisplayTasks[0].ExecTasks, 3)
s.Contains(vt.DisplayTasks[0].ExecTasks, "9001_task")
s.Contains(vt.DisplayTasks[0].ExecTasks, "very_task")
s.Contains(vt.DisplayTasks[0].ExecTasks, "another_disabled_task")
} else if vt.Variant == "bv_2" {
s.Len(vt.Tasks, 1)
s.Contains(vt.Tasks, "a_task_2")
Expand Down
25 changes: 25 additions & 0 deletions rest/model/patch.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ type ChildPatch struct {
PatchID *string `json:"patch_id"`
Status *string `json:"status"`
}

type VariantTask struct {
// Name of build variant
Name *string `json:"name"`
Expand Down Expand Up @@ -149,6 +150,7 @@ type APIPatchArgs struct {
}

// BuildFromService converts from service level structs to an APIPatch.
// An APIPatch expects the VariantTasks to be populated with only non-execution tasks and display tasks.
// If args are set, includes identifier, commit queue position, and/or child patches from the DB, if applicable.
func (apiPatch *APIPatch) BuildFromService(p patch.Patch, args *APIPatchArgs) error {
apiPatch.buildBasePatch(p)
Expand Down Expand Up @@ -242,16 +244,39 @@ func (apiPatch *APIPatch) buildBasePatch(p patch.Patch) {
}
apiPatch.Tasks = tasks
variantTasks := []VariantTask{}

// We remove the execution tasks from selected display tasks to avoid duplication.
execTasksToRemove := []string{}
for _, vt := range p.VariantsTasks {
vtasks := make([]*string, 0)
for _, task := range vt.Tasks {
vtasks = append(vtasks, utility.ToStringPtr(task))
}
for _, task := range vt.DisplayTasks {
vtasks = append(vtasks, utility.ToStringPtr(task.Name))
execTasksToRemove = append(execTasksToRemove, task.ExecTasks...)
}
variantTasks = append(variantTasks, VariantTask{
Name: utility.ToStringPtr(vt.Variant),
Tasks: vtasks,
})
}
for i, vt := range variantTasks {
tasks := []*string{}
for j, t := range vt.Tasks {
keepTask := true
for _, task := range execTasksToRemove {
if utility.FromStringPtr(t) == task {
keepTask = false
break
}
}
if keepTask {
tasks = append(tasks, vt.Tasks[j])
}
}
variantTasks[i].Tasks = tasks
}
apiPatch.VariantsTasks = variantTasks
apiPatch.Activated = p.Activated
apiPatch.Alias = utility.ToStringPtr(p.Alias)
Expand Down
43 changes: 43 additions & 0 deletions rest/model/patch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,3 +224,46 @@ func TestDownstreamTasks(t *testing.T) {
assert.Len(a.DownstreamTasks[0].Tasks, 2)
assert.Len(a.DownstreamTasks[0].VariantTasks, 1)
}

func TestPreselectedDisplayTasks(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth testing how it behaves where the user only selects a subset of execution tasks in the display task? I'm not sure if anyone really selects just a subset of a display task to run, but I don't think it's disallowed either, and may count as surprising/unexpected behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The execution tasks shouldn't be allowed to select on the UI (though I think the configure route could get behind that).

This test I think tests that though, it selects an execution tasks and makes sure the corresponding display task and other execution tasks are also paired (which propagates up to the variant tasks)

require.NoError(t, db.ClearCollections(patch.Collection, model.ProjectRefCollection))

p := patch.Patch{
Id: mgobson.NewObjectId(),
Description: "test",
Project: "mci",
Tasks: []string{"variant_task_1", "variant_task_2", "exec1", "exec2"},
VariantsTasks: []patch.VariantTasks{
{
Variant: "coverage",
Tasks: []string{"variant_task_1", "variant_task_2", "exec1", "exec2"},
DisplayTasks: []patch.DisplayTask{
{
Name: "display_task",
ExecTasks: []string{"exec1", "exec2"},
},
},
},
},
}
require.NoError(t, p.Insert())

a := APIPatch{}
err := a.BuildFromService(p, nil)
require.NoError(t, err)

// We expect the tasks from the patch to be only non-execution tasks + display tasks.
require.Len(t, a.VariantsTasks, 1)
assert.Len(t, a.VariantsTasks[0].Tasks, 3)

tasks := []string{}
for _, task := range a.VariantsTasks[0].Tasks {
tasks = append(tasks, utility.FromStringPtr(task))
}

assert.Contains(t, tasks, "variant_task_1")
assert.Contains(t, tasks, "variant_task_2")
assert.NotContains(t, tasks, "exec1")
assert.NotContains(t, tasks, "exec2")
assert.Contains(t, tasks, "display_task")
}
Loading