Skip to content

local: do not run serve_cmd with auto_init=False on start#4573

Merged
milas merged 2 commits into
masterfrom
milas/local-serve-only-manual-init
May 21, 2021
Merged

local: do not run serve_cmd with auto_init=False on start#4573
milas merged 2 commits into
masterfrom
milas/local-serve-only-manual-init

Conversation

@milas

@milas milas commented May 20, 2021

Copy link
Copy Markdown
Contributor

If a local_resource had no update_cmd (i.e. just a serve_cmd)
but was set to auto_init=False, it would end up starting anyway
because the server controller saw that the dependency status was
not_applicable (technically true).

To fix this, the dependency status is set to none for local
resources with auto_init=False that have not yet had a build
(either from being manually triggered or having TRIGGER_MODE_AUTO
and a file change). Once a build has happened, it will resume
returning not_applicable to indicate that there is no longer a
reason to block on a dependency.

Fixes #4548.

If a `local_resource` had no `update_cmd` (i.e. just a `serve_cmd`)
but was set to `auto_init=False`, it would end up starting anyway
because the server controller saw that the dependency status was
`not_applicable` (technically true).

To fix this, the dependency status is set to `none` for local
resources with `auto_init=False` that have not yet had a build
(either from being manually triggered or having `TRIGGER_MODE_AUTO`
and a file change). Once a build has happened, it will resume
returning `not_applicable` to indicate that there is no longer a
reason to block on a dependency.

Fixes #4548.
@milas milas added the bug Something isn't working label May 20, 2021
@milas
milas requested review from landism and nicks May 20, 2021 15:35
@milas milas changed the title local: do not run serve_cmd on auto_init=False local: do not run serve_cmd at start on auto_init=False May 20, 2021
@milas milas changed the title local: do not run serve_cmd at start on auto_init=False local: do not run serve_cmd with auto_init=False on start May 20, 2021
Comment thread internal/store/manifest_target.go Outdated
// Once manually triggered, a no-op build will exist, and subsequent calls will return
// UpdateStatusNotApplicable so that the server controller knows it does not need to
// wait for anything.
if !m.TriggerMode.AutoInitial() && !mt.State.StartedFirstBuild() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmmm...i'm not sure this is right. a couple obvious smells that leap out to me:

  • Currently mt.State.UpdateStatus() does this same check. why does this check live in both places?
  • Other things read UpdateStatus too (like the UI), will they also do the right thing in this case?
  • I'm also not sure I can articulate the difference between "UpdateStatusNone" and "UpdateStatusNotApplicable". can you?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

a check that might make more sense to me here is

	if us == v1alpha1.UpdateStatusNone {
		return us
	}

which is closer to the check above

@milas milas May 20, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I simplified the logic so that it only explicitly handles the special cases -- there's technically slight semantic change here now which is that UpdateStatusError won't get coerced to UpdateStatusNotApplicable anymore, but I think that's a good thing because that'd be indicative of an engine bug (i.e. no-op build "failing" should be impossible) and would probably result in UI being in a confusing/inconsistent state.

RE: The approach, I agree it's odd. IMO while it is kludgy it's at least consistent vs a non-existent update being allowed to be Pending but not None, i.e. it should either always be NotApplicable or it should generally follow the rules. (FWIW I'm also a bit confused why it coerces InProgress -> Pending instead of making InProgress a "hold" status in server controller, but that's a separate issue.)

Alternatively, we can add special logic to server controller for N/A statuses to do a !m.TriggerMode.AutoInitial() && !mt.State.StartedFirstBuild() check?

For difference between None / NotApplicable -- I agree, I think the name is unfortunate but the docs/usage are consistent since it's what gets used for anything that's auto_init=False:

// This resource hasn't had any reason to update yet.
// This usually indicates that it's a manual trigger with no auto_init.
UpdateStatusNone UpdateStatus = "none"

Thankfully the UI does handle it appropriately (also confirmed by actually running):

tilt/web/src/status.tsx

Lines 12 to 15 in 87a7f57

} else if (
res.updateStatus == UpdateStatus.NotApplicable ||
res.updateStatus == UpdateStatus.None
) {

@nicks

nicks commented May 20, 2021

Copy link
Copy Markdown
Contributor

ya, i like this approach!

@milas
milas merged commit cd47a42 into master May 21, 2021
@milas
milas deleted the milas/local-serve-only-manual-init branch May 21, 2021 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False doesn't work for serve_cmd-only resources

2 participants