Add `--stdin` for `stdin` log parsing by hogklint · Pull Request #292 · stern/stern · GitHub
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

Add --stdin for stdin log parsing #292

Merged
merged 6 commits into from
Mar 29, 2024
Merged

Add --stdin for stdin log parsing #292

merged 6 commits into from
Mar 29, 2024

Conversation

hogklint
Copy link
Contributor

@hogklint hogklint commented Mar 20, 2024

Usage:

$ kubectl logs <pod> | stern --stdin
$ stern --stdin < ./service.log

This PR adds the --stdin flag for parsing logs from stdin instead of pulling them from k8s. It creates a new "tail" struct, FileTail, which is heavily based on the original Tail. The new class is quite slim and I didn't find any meaningful way to combine the two, so they are mostly their own separate thing. They do share some helper functions and TailOptions which I moved to a "utils" file.

It's not uncommon for CLI tools to use - to read from stdin but I didn't find support for that in pflag, and I'm not sure it's better than --stdin anyway. (e.g like vim echo "Hello" | vim -, and for stern stern - < ./service.log)

Feedback welcome.

Implements #291

When Log and TailOptions are used by other classes it is now more
obvious that they're not hard lined to Tail.
FileTail is heavily based on Tail but I saw no obvious way to abstract
the common parts of the classes to not duplicate them. FileTail is so
small it's hardly noticeable anyway.
Copy link
Contributor

@tksm tksm left a comment

Choose a reason for hiding this comment

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

I'm sorry for the delayed reply.

This feature will also be great for testing templates 👍. I would like to incorporate this feature, but I would like to confirm that at least one other maintainer agrees before proceeding. @floryut @rkmathi @superbrothers What do you think?

Thanks for the PR!

cmd/cmd.go Outdated Show resolved Hide resolved
stern/file_tail.go Outdated Show resolved Hide resolved
stern/file_tail.go Show resolved Hide resolved
stern/stern.go Outdated Show resolved Hide resolved
Also moving file tailing to main thread as there are no parallel tails
with that config.
Copy link
Contributor

@tksm tksm left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@rkmathi rkmathi left a comment

Choose a reason for hiding this comment

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

Sorry for the late confirmation.
I think this feature is very useful 👍

@tksm tksm merged commit 53fc746 into stern:master Mar 29, 2024
2 checks passed
@hogklint
Copy link
Contributor Author

Awesome, thanks folks!

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