Skip to content

engine: docker-compose manifest and global logs respect the same filters#919

Merged
jazzdan merged 4 commits into
masterfrom
dmiller/ch1205/docker-compose-global-logs-dont-respect-filters
Jan 4, 2019
Merged

engine: docker-compose manifest and global logs respect the same filters#919
jazzdan merged 4 commits into
masterfrom
dmiller/ch1205/docker-compose-global-logs-dont-respect-filters

Conversation

@jazzdan

@jazzdan jazzdan commented Jan 4, 2019

Copy link
Copy Markdown
Contributor

No description provided.

@jazzdan
jazzdan requested review from maiamcc and nicks January 4, 2019 16:25

@nicks nicks 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.

hmmm...i don't see what this has to do with the attached clubhouse ticket?

var _ store.Subscriber = &DockerComposeLogManager{}

func shouldFilterDCLog(p []byte) bool {
if bytes.Contains(p, []byte("Attaching to")) {

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.

this seems way too broad? and will catch lots of legit things.

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 should be a "starts with" check?

@jazzdan

jazzdan commented Jan 4, 2019

Copy link
Copy Markdown
Contributor Author

@nicks sorry I should have made more clear: I saw this as a TODO in the code and decided to fix it as I re-familiarize myself with the log code.

@jazzdan
jazzdan force-pushed the dmiller/ch1205/docker-compose-global-logs-dont-respect-filters branch 5 times, most recently from b809b52 to 58339a6 Compare January 4, 2019 17:48
@jazzdan
jazzdan force-pushed the dmiller/ch1205/docker-compose-global-logs-dont-respect-filters branch from 58339a6 to 1d98151 Compare January 4, 2019 18:43

@maiamcc maiamcc 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.

nice!

Comment thread internal/build/test_utils.go Outdated

err = stdout.WaitUntilContains("listening on", 5*time.Second)
if isCircleCI() {
err = stderr.WaitUntilContains("listening on", 5*time.Second)

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.

womp womp. but legit.

I think i'd prefer combinedOut.WaitUntilContains and then we wouldn't need this conditional -- any way to do that?

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.

Check it out now!

@jazzdan
jazzdan merged commit bb77634 into master Jan 4, 2019
cmd := exec.Command("docker", "run", "--name", "tilt-registry", "-p", "5005:5000", "registry:2")
cmd.Stdout = stdout
cmd.Stderr = stderr
cmd.Stderr = stdout

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.

this should probs have a comment?

@nicks
nicks deleted the dmiller/ch1205/docker-compose-global-logs-dont-respect-filters branch March 31, 2021 16:06
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