-
Notifications
You must be signed in to change notification settings - Fork 488
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
Loki crds #3936
Loki crds #3936
Conversation
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.
Thanks for the PR!
This LGTM at a first glance with a few nits, but I haven't tested this yet and would like to before we check it in.
HTTPClientConfig: config.DefaultHTTPClientConfig, | ||
} | ||
|
||
func (args *Arguments) UnmarshalRiver(f func(interface{}) error) error { |
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.
We should make use of the new river.Validator and river. Defaulter interfaces if possible here.
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.
Some suggestions for the docs
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
Hey @EStork09 how's it going? 👋 Would you have the time to look through the docs suggestions and rebase so we can move ahead with this? |
@tpaschalis sorry, I have just recently relocated to Europe, I will try to get to this when I can |
Signed-off-by: Paschalis Tsilias <paschalist0@gmail.com>
…pdated interface Signed-off-by: Paschalis Tsilias <paschalist0@gmail.com>
Signed-off-by: Paschalis Tsilias <paschalist0@gmail.com>
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.
Small cleanup of a few agent
that slipped through the first review
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
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.
Additional docs fixes
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
To avoid duplicating so much code it should be possible to put some in the common kubernetes pkg. The client part could also use a common pkg. But we can do this via a follow-up PR and focus on getting this one merged |
I tested it and it works: I created a local k8s cluster with the agent using the helm chart and the prometheus operator. Then I created the following rule:
and with this component:
|
docs/sources/flow/reference/components/loki.rules.kubernetes.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
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.
Made some little changes and updated with fresh main. Retested after the changes and everything works.
Thansk again @EStork09 and sorry it took so long! |
PR Description
Which issue(s) this PR fixes
Fixes #3395
Notes to the Reviewer
Copied #2604 basically.
Would remove the 404 skip check when #9489 closes on the loki repo.
Had to use
-
to seperate rule groups/namespaces,/
was not properly passing through the routes, it appears loki needs to align with mimir to handle those, loki is just using Path while mimir is using PathPrefix.PR Checklist