Skip to content

tilt logs: add port/resource flags, filter by resource [ch8432 ch8433]#3655

Merged
maiamcc merged 2 commits into
masterfrom
maiamcc/filter-resources
Aug 6, 2020
Merged

tilt logs: add port/resource flags, filter by resource [ch8432 ch8433]#3655
maiamcc merged 2 commits into
masterfrom
maiamcc/filter-resources

Conversation

@maiamcc

@maiamcc maiamcc commented Aug 3, 2020

Copy link
Copy Markdown
Contributor

Hello @nicks, @jazzdan,

Please review the following commits I made in branch maiamcc/filter-resources:

857bdf0 (2020-08-03 13:03:31 -0400)
tilt logs: add port/resource flags, filter by resource [ch8432 ch8433]

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@maiamcc
maiamcc requested review from jazzdan and nicks August 3, 2020 17:06
Comment thread internal/hud/server/logs_reader.go Outdated
if toCheckpoint > ls.checkpoint {
ls.checkpoint = toCheckpoint
if segsAdded > 0 {
ls.checkpointInternal += logstore.Checkpoint(segsAdded)

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.

shouldn't this be ls.logstore.Checkpoint(), like we do in TerminalStream?

no reason to be doing checkpoint arithmetic out here

Comment thread internal/hud/server/logs_reader.go Outdated

// TODO(maia): filter for the resources that we care about (`tilt logs resourceA resourceC`)
// --> and if there's only one resource, don't prefix logs with resource name?
// TODO(maia): if there's only one resource, don't prefix logs with resource name

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.

i wonder if it makes more sense to have an options object to ContinuingLines that takes a set of resources to filter on....we already do this internally with toLogLines logOptions (which filters on a set of spans)

@nicks

nicks commented Aug 3, 2020

Copy link
Copy Markdown
Contributor

also - the PR description says it added port flags, but i couldn't find them?

@maiamcc
maiamcc force-pushed the maiamcc/filter-resources branch from 857bdf0 to 3a7b455 Compare August 6, 2020 20:02
@maiamcc
maiamcc requested a review from nicks August 6, 2020 20:02
@maiamcc

maiamcc commented Aug 6, 2020

Copy link
Copy Markdown
Contributor Author

reworked this with new code from #3664, much simpler now, ty @nicks! Nick and @jazzdan PTAL?

var alphabet = []string{"alpha", "bravo", "charlie", "delta", "echo", "foxtrot",
"golf", "hotel", "igloo", "juliette", "kilo", "lima", "mike", "november",
"oscar", "papa", "quebec", "romeo", "sierra", "tango", "uniform", "victor",
"whiskey", "xavier", "yankee", "zulu"}

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.

🥃

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.

cheers!

@maiamcc
maiamcc merged commit 27576b7 into master Aug 6, 2020
@maiamcc
maiamcc deleted the maiamcc/filter-resources branch August 6, 2020 21:37
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