engine: trigger mode auto without inital #4292

maiamcc · 2021-03-09T18:08:05Z

Allow TRIGGER_MODE_AUTO with auto_init=False, i.e. resources that update automatically in response to changes in watched files, but do NOT automatically update when you first start Tilt.

I was dogfooding tests and trying the approach of "one resource per test file/test suite" and getting super frustrated waiting for them all to run at once--here's a solution where only the test resources I need will run (...assuming I get dependency logic right).

I know i'm kind of cheating with the golang constant names, suggestions welcome.

Also not positive what i should do with the trigger mode toggle logic in the FE, but this seems like an ok first guess?

nicks · 2021-03-09T18:53:19Z

After this change, is there still a strong reason to group "AutoInit" and TriggerMode together into one identifier? now that they're orthogonal, it doesn't seem like this makes sense anymore.

nicks · 2021-03-09T18:56:45Z

Particularly worried about this given that in the new world (with @milas 's FileWatch stuff), "TriggerModeAuto" is just a FileWatch object, and has nothing to do with auto-init...though I haven't totally thought through how auto_init is represented in the new data model.

maiamcc · 2021-03-09T19:07:29Z

After this change, is there still a strong reason to group "AutoInit" and TriggerMode together into one identifier? now that they're orthogonal, it doesn't seem like this makes sense anymore.

Off the top of my head no, no reason to keep them lumped together like this--you're right that it makes more sense to store them as two separate things. (@landism could speak more to why we grouped them together in the backend in the first place?)

Is this a "consideration for the future" or do you think figuring out the next step of the data model should block this PR? To me, this change feels like an immediate benefit that doesn't make it any harder for us to refactor things in future, but I'm willing to be wrong.

nicks · 2021-03-09T19:22:30Z

how hard would it be to split them into separate fields in this PR? as you pointed out in your original comment, it seems like the PR is already going through some contortions / weird names to keep them together...

maiamcc · 2021-03-09T19:47:50Z

Like, pretty easy but not trivial if I want to get all the testing right

nicks · 2021-03-09T21:15:37Z

internal/engine/buildcontroller.go

@@ -211,7 +211,7 @@ func buildStateSet(ctx context.Context, manifest model.Manifest, specs []model.T

 	isLiveUpdateEligibleTrigger := reason.HasTrigger() &&
 		reason.Has(model.BuildReasonFlagChangedFiles) &&
-		manifest.TriggerMode != model.TriggerModeAuto
+		manifest.TriggerMode != model.TriggerModeAuto_AutoInit


i think this needs to include all TriggerMode=auto?

nicks · 2021-03-09T21:16:38Z

internal/store/engine_state.go

@@ -681,7 +681,7 @@ func (ms *ManifestState) UpdateStatus(triggerMode model.TriggerMode) model.Updat
 	hasPendingBuild := false
 	if ms.TriggerReason != 0 {
 		hasPendingBuild = true
-	} else if triggerMode == model.TriggerModeAuto && hasPendingChanges {
+	} else if triggerMode == model.TriggerModeAuto_AutoInit && hasPendingChanges {


i think this needs to include all auto?

nicks · 2021-03-09T21:22:14Z

pkg/model/trigger_mode.go

@@ -11,26 +11,29 @@ type TriggerMode int
 //      for an update
 const (
 	// Tilt automatically performs initial and non-initial builds without manual intervention
-	TriggerModeAuto TriggerMode = iota
+	TriggerModeAuto_AutoInit TriggerMode = iota


i thought Golang names didn't have underscores, cf https://golang.org/doc/effective_go#mixed-caps

maybe call them

TriggerOnManualOnly
TriggerOnFileChange
TriggerOnInit
TriggerOnFileChangeOrInit

?

I mean they're not supposed to have underscores, no, that's why I said i was cheating 😅

hmmm...were you going to change the names?

oh whoops i missed this. fixing now.

nicks · 2021-03-12T01:43:27Z

high-level question that came up when I was talking to another team about this:

My understanding is that we want to mark tests as trigger-mode=manual and auto-init=false. How does this play out in CI mode?

Seems like you'd want ci mode to run the tests in that case

maiamcc · 2021-03-22T20:12:23Z

My understanding is that we want to mark tests as trigger-mode=manual and auto-init=false

How do you figure? I would assume trigger-mode=auto, and auto-init honestly depends on a given user's preference so I would just leave it as the default (which is =true).

Seems like you'd want ci mode to run the tests in that case

I mean, depends what you're using CI mode for--if you just want to make sure your k8s configs are correct and your services stand up right, you don't (necessarily) care about test results. I'd err on the side of not changing default behaviors -- if people need to, they can set auto_init=config.tilt_subcommand=='ci'.

maiamcc · 2021-03-22T20:16:02Z

all that said, if you're worried, we don't have to merge this. I think it'll make the experience of tests a lot nicer, but we can also wait to hear that from more users, or take longer to plan this out.

nicks · 2021-03-22T20:38:55Z

nah, if this makes the test experience much nicer, let's move forward with it.

i think you're misunderstanding my above comment - in the current PR, if you have a test with trigger-mode=auto and auto-init=false, and you run tilt ci, the ci command will hang forever. is that really what you want?

maiamcc · 2021-03-23T19:44:16Z

i think you're misunderstanding my above comment - in the current PR, if you have a test with trigger-mode=auto and auto-init=false, and you run tilt ci, the ci command will hang forever. is that really what you want?

Ah, thought this was a high-level behavior comment. Fixed.

nicks · 2021-03-23T21:08:56Z

pkg/model/trigger_mode.go

@@ -11,26 +11,29 @@ type TriggerMode int
 //      for an update
 const (
 	// Tilt automatically performs initial and non-initial builds without manual intervention
-	TriggerModeAuto TriggerMode = iota
+	TriggerModeAuto_AutoInit TriggerMode = iota


hmmm...were you going to change the names?

rename trigger modes to not use underscores

Previously, you could not have a resource that had `auto_init=False` with `trigger_mode=TRIGGER_MODE_AUTO`. This restriction was lifted in v0.19.1 a while back by tilt-dev/tilt#4292. This removes mention of the restriction from the docs including retconning a blog post from 2019.

Maia added 2 commits March 9, 2021 12:51

go changes

ba7f5dc

web changes/tests

d7b6534

maiamcc requested review from nicks and landism March 9, 2021 18:08

Merge branch 'master' into maiamcc/auto-after-initial

35b75d8

nicks reviewed Mar 9, 2021

View reviewed changes

Merge branch 'master' into maiamcc/auto-after-initial

c4c8f85

Maia added 2 commits March 23, 2021 12:37

code review

beaed91

correct behavior for CI mode

8ed4a84

maiamcc requested a review from nicks March 23, 2021 21:01

nicks approved these changes Mar 23, 2021

View reviewed changes

rename to remove underscores

bd51486

rename trigger modes to not use underscores

maiamcc merged commit 181592a into master Mar 23, 2021

maiamcc deleted the maiamcc/auto-after-initial branch March 23, 2021 21:38

milas mentioned this pull request Jun 1, 2021

resources that trigger on file change but don't auto-init #4275

Closed

milas mentioned this pull request Jun 9, 2021

docs: remove auto_init=False / TRIGGER_MODE_AUTO restrictions tilt-dev/tilt.build#868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

engine: trigger mode auto without inital #4292

engine: trigger mode auto without inital #4292

maiamcc commented Mar 9, 2021

nicks commented Mar 9, 2021

nicks commented Mar 9, 2021

maiamcc commented Mar 9, 2021 •

edited

nicks commented Mar 9, 2021

maiamcc commented Mar 9, 2021

nicks Mar 9, 2021

nicks Mar 9, 2021

nicks Mar 9, 2021

maiamcc Mar 9, 2021

nicks Mar 23, 2021

maiamcc Mar 23, 2021

nicks commented Mar 12, 2021

maiamcc commented Mar 22, 2021

maiamcc commented Mar 22, 2021

nicks commented Mar 22, 2021

maiamcc commented Mar 23, 2021

nicks Mar 23, 2021

engine: trigger mode auto without inital #4292

engine: trigger mode auto without inital #4292

Conversation

maiamcc commented Mar 9, 2021

nicks commented Mar 9, 2021

nicks commented Mar 9, 2021

maiamcc commented Mar 9, 2021 • edited

nicks commented Mar 9, 2021

maiamcc commented Mar 9, 2021

nicks Mar 9, 2021

Choose a reason for hiding this comment

nicks Mar 9, 2021

Choose a reason for hiding this comment

nicks Mar 9, 2021

Choose a reason for hiding this comment

maiamcc Mar 9, 2021

Choose a reason for hiding this comment

nicks Mar 23, 2021

Choose a reason for hiding this comment

maiamcc Mar 23, 2021

Choose a reason for hiding this comment

nicks commented Mar 12, 2021

maiamcc commented Mar 22, 2021

maiamcc commented Mar 22, 2021

nicks commented Mar 22, 2021

maiamcc commented Mar 23, 2021

nicks Mar 23, 2021

Choose a reason for hiding this comment

maiamcc commented Mar 9, 2021 •

edited