engine: roll forward of https://github.com/tilt-dev/tilt/pull/3362, re-using image builds from other resources #3553
Conversation
|
Does this want REVIEW or is it a revert-revert? |
|
review!
|
internal/engine/upper.go
Outdated
| // | ||
| // 2) If the current manifest build was kicked off by a trigger, we don't | ||
| // want to queue manifests with the same image. | ||
| if currentMT.NextBuildReason() == model.BuildReasonNone { |
There was a problem hiding this comment.
nbd, but this might be more readable as a helper function a la if !currentMT.IsQueued() or something.
There was a problem hiding this comment.
understand the instinct here, but this particular check on has that particular meaning in this particular context -- it would not be correct if called from anywhere
internal/engine/upper.go
Outdated
|
|
||
| // Update build statuses for duplicated image targets in other manifests. | ||
| // This ensures that those images targets aren't redundantly rebuilt. | ||
| for _, currentMT := range engineState.Targets() { |
There was a problem hiding this comment.
nbd but consider a helper function here for readability: for... := range engineState.TargetsBesides(mn)
| // builds. | ||
| // | ||
| // 2) If the current manifest build was kicked off by a trigger, we don't | ||
| // want to queue manifests with the same image. |
| // We need to mark imageB as dirty, because it was not built in the manifestA | ||
| // build but its dependency was built. | ||
| // | ||
| // Note that this logic also applies to deploy targets depending on image |
| // targets. If we built manifestA, which contains imageX and deploy target | ||
| // k8sA, and manifestB contains imageX and deploy target k8sB, we need to mark | ||
| // target k8sB as dirty so that Tilt actually deploys the changes to imageX. | ||
| rDepsMap := currentMT.Manifest.ReverseDependencyIDs() |
There was a problem hiding this comment.
this is convoluted enough that i might comment every line:
| // target k8sB as dirty so that Tilt actually deploys the changes to imageX. | ||
| rDepsMap := currentMT.Manifest.ReverseDependencyIDs() | ||
| for updatedID := range updatedIDSet { | ||
| for _, rDepID := range rDepsMap[updatedID] { |
There was a problem hiding this comment.
// for each target depending on each target we just updated...
| rDepsMap := currentMT.Manifest.ReverseDependencyIDs() | ||
| for updatedID := range updatedIDSet { | ||
| for _, rDepID := range rDepsMap[updatedID] { | ||
| if updatedIDSet[rDepID] { |
There was a problem hiding this comment.
// if that target itself was just updated, nothing to do
| continue | ||
| } | ||
|
|
||
| currentMS.MutableBuildStatus(rDepID).PendingDependencyChanges[updatedID] = br.StartTime |
There was a problem hiding this comment.
// otherwise, mark that target as dirty so it gets rebuilt
internal/engine/upper_test.go
Outdated
| state := call.state[m1.ImageTargets[0].ID()] | ||
| assert.Equal(t, map[string]bool{aPath: true}, state.FilesChangedSet) | ||
|
|
||
| // Make sure that when the second build is triggeredd, we did the bookkeeping |
There was a problem hiding this comment.
| // Make sure that when the second build is triggeredd, we did the bookkeeping | |
| // Make sure that when the second build is triggered, we did the bookkeeping |
| state := call.state[m1.ImageTargets[0].ID()] | ||
| assert.Equal(t, map[string]bool{aPath: true}, state.FilesChangedSet) | ||
|
|
||
| // Make sure that when the second build is trigged, we did the bookkeeping |
There was a problem hiding this comment.
| // Make sure that when the second build is trigged, we did the bookkeeping | |
| // Make sure that when the second build is triggered, we did the bookkeeping |
Hello @maiamcc, @landism,
Please review the following commits I made in branch nicks/ch6738:
24351f9 (2020-07-06 11:43:41 -0400)
engine: roll forward of #3362, re-using image builds from other resources
Code review reminders, by giving a LGTM you attest that: