-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Support series API in logcli #1861
Conversation
43fb03e
to
6f798fb
Compare
Codecov Report
@@ Coverage Diff @@
## master #1861 +/- ##
==========================================
+ Coverage 64.84% 64.89% +0.05%
==========================================
Files 125 125
Lines 9423 9426 +3
==========================================
+ Hits 6110 6117 +7
+ Misses 2886 2885 -1
+ Partials 427 424 -3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yeya24 Thank you so much !! It look awesome. And thanks for fixing many typos and formatting issue you deserve a medal 🏅
LGTM
If you have some time to rebase and fix the conflict, I'll get that merged asap. |
Signed-off-by: yeya24 <yb532204897@gmail.com> add changelog Signed-off-by: yeya24 <yb532204897@gmail.com>
Hi @cyriltovena @owen-d I have rebased my code. PTAL |
cmd.Flag("since", "Lookback window.").Default("1h").DurationVar(&since) | ||
cmd.Flag("from", "Start looking for logs at this absolute time (inclusive)").StringVar(&from) | ||
cmd.Flag("to", "Stop looking for logs at this absolute time (exclusive)").StringVar(&to) | ||
cmd.Flag("match", "eg '{foo=\"bar\",baz=~\".*blip\"}'").Required().StringsVar(&q.Matchers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show me an example of how it work with multiple matches ? what's the cli look like for them ? is it space based or comma seperated ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking cause I see that StringsVar
is an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also like to know that, but otherwise LGTM!
Nice work @yeya24. Would you mind updating https://github.com/grafana/loki/blob/master/docs/getting-started/logcli.md?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I won't hold up the PR if you don't want to add the docs, but I'd appreciate it :). Let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add it this afternoon. Will ping you when I finish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I have updated the docs. PTAL @owen-d
Signed-off-by: yeya24 <yb532204897@gmail.com>
Signed-off-by: yeya24 yb532204897@gmail.com
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1841
Special notes for your reviewer:
Checklist