Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

engine: add status for PodLogStream #4395

Merged
merged 1 commit into from Apr 5, 2021
Merged

engine: add status for PodLogStream #4395

merged 1 commit into from Apr 5, 2021

Conversation

nicks
Copy link
Member

@nicks nicks commented Apr 1, 2021

Hello @milas, @maiamcc,

Please review the following commits I made in branch nicks/ch11770:

a47119e (2021-04-01 17:39:37 -0400)
engine: add status for PodLogStream

8fef11a (2021-03-31 19:59:31 -0400)
engine: make podlogmanager a reconciler

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

@nicks nicks requested review from maiamcc and milas April 1, 2021 21:43
@shortcut-integration
Copy link

This pull request has been linked to Clubhouse Story #11770: write PodLogStream status.

@nicks nicks changed the base branch from master to nicks/ch11687 April 1, 2021 21:43
@nicks
Copy link
Member Author

nicks commented Apr 1, 2021

I also feel like I'm missing an easy abstraction for queuing and updating the API server with status updates...i haven't spent much time looking at how Kubernetes code does this...

@nicks nicks force-pushed the nicks/ch11770 branch 3 times, most recently from bfb9fed to 5085f99 Compare April 1, 2021 22:58
@nicks nicks changed the base branch from nicks/ch11687 to master April 2, 2021 15:05
@nicks nicks changed the base branch from master to nicks/ch11687 April 2, 2021 15:05
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

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

Looks good to me once it gets tilt-dev/tilt-apiserver#29 rolled in

Base automatically changed from nicks/ch11687 to master April 5, 2021 15:58
@nicks nicks merged commit 8aeb180 into master Apr 5, 2021
@nicks nicks deleted the nicks/ch11770 branch April 5, 2021 16:21
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.

None yet

2 participants