Skip to content

engine: record and display Tiltfile logs separately#1050

Merged
jazzdan merged 6 commits into
masterfrom
dmiller/ch1343/add-tiltfile-item-with-execution-log
Jan 24, 2019
Merged

engine: record and display Tiltfile logs separately#1050
jazzdan merged 6 commits into
masterfrom
dmiller/ch1343/add-tiltfile-item-with-execution-log

Conversation

@jazzdan

@jazzdan jazzdan commented Jan 18, 2019

Copy link
Copy Markdown
Contributor

Regular (good) view:

regular view

There's a Tiltfile and an error:

screen shot 2019-01-18 at 1 46 01 pm

There's no Tiltfile:

screen shot 2019-01-18 at 1 46 29 pm

@jazzdan
jazzdan requested review from hyu and landism January 18, 2019 18:47
@jazzdan

jazzdan commented Jan 18, 2019

Copy link
Copy Markdown
Contributor Author

This is just a first pass. There's still some rough edges in the UI: build history, and the color of the dot being the ones that are foremost in my mind right now.

Comment thread internal/engine/upper.go
}

manifests, globalYAML, configFiles, err := tiltfile.Load(ctx, fileName, matching)
// TODO(dmiller) hmm, can I do this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

?

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.

Like can I just pass u.store in to this, or will that cause problems?

}

manifests, globalYAML, configFiles, err := tiltfile.Load(ctx, tiltfilePath, matching)
tlw := NewTiltfileLogWriter(st)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we also want to fork the logger in ctx so that anything that writes to the logger makes it into the tiltfile log.

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.

I don't think that's true, we want them to be two separate streams.

Comment thread internal/store/engine_state.go Outdated

IsProfiling bool

TiltfileLog []byte `testdiff:"ignore"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should probably be CurrentTiltfileBuild model.BuildRecord (which then also takes care of your "we might not want to grab this build" TODO)

Comment thread internal/hud/renderer.go Outdated
}

func bestLogs(res view.Resource) string {
if res.IsTiltfile && res.CrashLog != "" {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I'd be surprised if it makes sense to have the crash log preempt the build log for tiltfiles.

I'd think we'd always want to show all the tiltfile output and any error that occurred, rather than only showing the error when there's an error, and then just not bother touching the CrashLog field.

The segregation between build and crash logs for k8s services is due to wanting to smooth things out across pod restarts, which isn't really relevant to tiltfile execution.

Or maybe I've misunderstood this behavior?

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.

Yeah this is a good point, let me experiment with this some more.

@jazzdan
jazzdan force-pushed the dmiller/ch1343/add-tiltfile-item-with-execution-log branch from 03fe4dc to f8c8ad4 Compare January 24, 2019 21:14

@hyu hyu left a comment

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.

LGTM in terms of the design of the feature!

@jazzdan
jazzdan merged commit 55106f2 into master Jan 24, 2019
@nicks
nicks deleted the dmiller/ch1343/add-tiltfile-item-with-execution-log branch March 31, 2021 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants