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.

Sign up for GitHub

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

Jump to bottom

Added support for init containers #15

Closed
wants to merge 2 commits into from
Closed

Added support for init containers #15

wants to merge 2 commits into from

Conversation

davidreuss
Copy link
Contributor

Init containers are considered, when their container status Running
field is non nil

Fixes #14

@davidreuss
Added support for init containers
ba868fb 
Init containers are considered, when their container status `Running`
field is non nil
@davidreuss davidreuss mentioned this pull request Nov 21, 2017
Closed
@boz
Copy link
Owner

boz commented Nov 21, 2017

This looks great. Thanks, @davidreuss!

I was worried about having to deal with these semantics:

The list has one entry per init container in the manifest. The most recent successful init container will have ready = true, the most recently started container will have startTime set.

But it looks like you handled it concisely by checking State.Running.

I'll give it a whirl shortly.

@davidreuss
Copy link
Contributor Author

Actually i think the Running check should be used for regular containers as well. At least when i use kail i do so for following any containers, and want logs output as soon as possible. With the check being hinged on Ready i only see logs when readiness probe passes, and there could be logging done before that (bootstrap/setup/whatever) by the container/app.

@boz
Copy link
Owner

boz commented Nov 22, 2017

@davidreuss : Good call. How about rolling it all up into Accept()? It'd be cleaner and would make --containers behave correctly (AcceptInitContainers() is letting init containers sneak through right now).

@davidreuss
Copy link
Contributor Author

Yep, thought as much, i’ll update the pr shortly.

@davidreuss
Use same running check, for all containers
23a43f2 
This also makes kail follow logs of unready containers, which is
desirable when you're depending on log output of containers that might
never enter ready state.
boz
boz requested changes Nov 23, 2017
View reviewed changes
filter.go
if name == cs.Name {
return true
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Just replace the !cs.Ready expression; the rest is used for filtering by container name with --container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, didn’t get that part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i won’t be near a computer until monday though, so it will have to wait sadly, but feel free to overtake this pr and put it back in, if you’d like, otherwise i’ll followup next week :)

@boz
Copy link
Owner

boz commented Nov 28, 2017

@davidreuss: I fixed up the patch a bit in 67ab5fd and set you as the author. Thanks for the contribution!

@boz boz closed this Nov 28, 2017
@davidreuss davidreuss deleted the init-containers branch November 28, 2017 06:28
@davidreuss
Copy link
Contributor Author

@boz was just about to pick this up again, but you beat me to it - thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boz boz boz requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@davidreuss @boz