-
Notifications
You must be signed in to change notification settings - Fork 176
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 support for missing objectstorage secret for thanos store #307
Conversation
Seems like lint/build is failing. Could you take a look @vprashar2929? |
Signed-off-by: Vibhu Prashar <vibhu.sharma2929@gmail.com>
Done @saswatamcode Now everything is green 😄 |
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.
So taking a deeper look here, I think we should do things a bit differently.
Let's not define any minio resources within manifests
but keep it to the examples/development-minio
dir instead, so as to not block any downstream user simply copying manifests. Also importing observatorium is potentially a cyclic import here.
Perhaps we can define a minio.libsonnet
jsonnet lib here, and generate the output of that in examples/development-minio
?
example.jsonnet
Outdated
@@ -1,12 +1,18 @@ | |||
local t = import 'kube-thanos/thanos.libsonnet'; | |||
local minio = (import 'github.com/observatorium/observatorium/configuration/components/minio.libsonnet')({ |
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.
Hmm. Maybe we can define minio.libsonnet
here?
Let's not create a cyclic import. Currently, observatorium/observatorium
also relies on kube-thanos as a dependency.
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.
Sure defining minio.libsonnet
here sounds good
manifests/minio-secret-thanos.yaml
Outdated
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.
So I don't think having this within manifests
is a good idea. Many downstream users simply want to copy over the manifests
YAML and define their own S3 secrets and parameters (directly skipping jsonnet generation).
Including the minio secret here, would lead to an extra step for them.
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.
Yes I think moving the minio-related resources to examples/development-minio
is the better way to proceed.
Signed-off-by: Vibhu Prashar <vibhu.sharma2929@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.
LGTM!
This PR: