Skip to content

engine: rm dead code in podLogManager#1252

Merged
maiamcc merged 2 commits into
masterfrom
maiamcc/clean-up-pod-log-watch
Mar 6, 2019
Merged

engine: rm dead code in podLogManager#1252
maiamcc merged 2 commits into
masterfrom
maiamcc/clean-up-pod-log-watch

Conversation

@maiamcc

@maiamcc maiamcc commented Mar 5, 2019

Copy link
Copy Markdown
Contributor

Hello @landism, @nicks,

Please review the following commits I made in branch maiamcc/clean-up-pod-log-watch:

c2fc560 (2019-03-05 17:52:39 -0500)
engine: rm dead code in podLogManager

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 landism and nicks March 5, 2019 22:52
}
}

func TestLogsTruncatedWhenCanceled(t *testing.T) {

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.

turns out this func didn't actually test anything

t.Fatal(err)
}

// Previous log stream has finished, so the first pod watch has been canceled,

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.

@landism make sense?

@maiamcc

maiamcc commented Mar 5, 2019

Copy link
Copy Markdown
Contributor Author

Rolling back a chunk of changes from #776 -- seems that all that was needed to fix the problem in question was the HardCancelReader

@landism landism left a comment

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 can also delete PodLogWatch.ContainerID?

@maiamcc
maiamcc merged commit f3bf242 into master Mar 6, 2019
@maiamcc
maiamcc deleted the maiamcc/clean-up-pod-log-watch branch March 6, 2019 16:04
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.

2 participants