DEVPROD-6266 Fix duplication for --repeat-patch #7892
Conversation
| // we can rely on them being there from the previous patch. | ||
| if failedOnly { | ||
| failedPlusNeeded, err := AddTasksNeededByFailedForReuse(failedTasks, failedTaskDisplayNames, project, vt) | ||
| if err != nil { |
There was a problem hiding this comment.
could we change "needed" language to dependency language? This is more consistent
There was a problem hiding this comment.
It needs both dependencies and tasks from single host task groups
There was a problem hiding this comment.
Ah. Perhaps "addDependenciesAndTaskGroups" then? I think right now the function includes details that really aren't relevant to the usage (that only failed tasks are passed in, and that its called by the reuse function).
Also, should we change the error message to say "dependencies and task groups"
There was a problem hiding this comment.
It looks like the first half of this message got missed -- I'd still suggest changing the function title and variables to be more specific about what's being done rather than why its being called.
There was a problem hiding this comment.
so strange, I did the change but it was somehow lost. My apologies, pushed it now.
units/patch_intent.go
Outdated
| } | ||
|
|
||
| // AddTasksNeededByFailedForReuse add tasks that failed tasks need to run including dependencies and tasks from single host task groups. | ||
| func AddTasksNeededByFailedForReuse(failedTasks []task.Task, failedTaskDisplayNames []string, project *model.Project, vt patch.VariantTasks) ([]string, error) { |
There was a problem hiding this comment.
can includeDependencies be used here instead, if we just convert the failedTasks to tv pairs?
(Also, i think this function can be private)
There was a problem hiding this comment.
I made it private. It also adds all tasks in a single host task group, so includeDependencies would only cover part of it.
units/patch_intent.go
Outdated
| func setToFilteredTasks(patchDoc, reusePatch *patch.Patch, project *model.Project, failedOnly bool) error { | ||
| activatedTasks, err := filterToActiveForReuse(reusePatch, project) | ||
| if err != nil { | ||
| return errors.Wrap(err, "getting dependencies for activated tasks") |
There was a problem hiding this comment.
this error message doesn't seem right
units/patch_intent.go
Outdated
| } | ||
|
|
||
| patchDoc.Tasks = failedTasks | ||
| if failedOnly { |
There was a problem hiding this comment.
Is this if statement necessary since filteredTasks = allFailedPlusNeededTasks at 691?
| // we can rely on them being there from the previous patch. | ||
| if failedOnly { | ||
| failedPlusNeeded, err := AddTasksNeededByFailedForReuse(failedTasks, failedTaskDisplayNames, project, vt) | ||
| if err != nil { |
There was a problem hiding this comment.
Ah. Perhaps "addDependenciesAndTaskGroups" then? I think right now the function includes details that really aren't relevant to the usage (that only failed tasks are passed in, and that its called by the reuse function).
Also, should we change the error message to say "dependencies and task groups"
units/patch_intent.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func filterToActiveForReuse(reusePatch *patch.Patch) ([]task.Task, error) { |
There was a problem hiding this comment.
I think this should be moved to task_db.go (doesn't need to reference reuse specifically, it's really just byActivatedTasksWithoutDisplay or something)
There was a problem hiding this comment.
Moving it creates an import cycle. It's specific enough to this use case that I think it's okay here
There was a problem hiding this comment.
Looks like this comment got missed
There was a problem hiding this comment.
I forgot to hit enter to submit all of my comments, sorry
There was a problem hiding this comment.
I think that's only because you're passing in the whole patch -- you can change this to just pass in the ID and then it should be fine
There was a problem hiding this comment.
(may want to do that anyway)
There was a problem hiding this comment.
LGTM with two missed comments (forgot to click save before publishing review, this is the other: #7892 (comment))
units/patch_intent.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func filterToActiveForReuse(reusePatch *patch.Patch) ([]task.Task, error) { |
There was a problem hiding this comment.
Looks like this comment got missed
DEVPROD-6266
Description
--repeat-patch was relying on BuildProjectTVPairs to create the variant tasks for patches. However, that resulted in a previous patch of: {bv1: t1, bv2: t2} ending up with {bv1: t1, bv1: t2, bv2: t1, bv2: t2}. This fixes that by reusing the variant tasks from the previous patch and applying the filtering on top of that instead of just on the tasks.
Testing
Existing unit tests and testing on staging.