-
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
*: Refactor to not mutate global objects #90
Conversation
82f6abe
to
3d5f095
Compare
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.
Skimming over it on mobile made me very excited. I'll give it another look on a computer after FOSDEM. 👍
Sounds good. I think I just had a better idea for persistent volume claim templates anyways :) |
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.
This looks great.
- I think we should fix naming of the components, as it's discussed in Move components names to
Ruler
,Compactor
,Querier
(-er
) thanos#1871.
receive
should be receiver
query
should be querier
- You also removed
sidecar
. Which is ok, I think. We didn't have anything significant in that file. However, it would be good to document this. And maybe we can add a helper function to add sidecars to queriers. Just for convenience. This could be handled in another PR, I just wanted to note it.
f808152
to
26a6684
Compare
@brancz My only concern about naming is inconsistency. I'm totally fine with keeping configuration names in sync with commands. In that case, we should rename My understanding from that issue (thanos-io/thanos#1871) was, we would eventually move to And it's more clear now. Just to recap we will keep structural things as in commands and only rename documentation, alert message etc. |
Looks like we have consensus in thanos-io/thanos#1871, I'll do the changes accordingly here. |
1a1e5e1
to
9c6a9ad
Compare
2b155ff
to
3fe7ba5
Compare
Signed-off-by: Frederic Branczyk <fbranczyk@gmail.com>
Here's my the progress on moving my personal Thanos cluster to this refactored kube-thanos version. https://gist.github.com/metalmatze/7280c8101024d0edbccd5fbedb82a2c1 |
Signed-off-by: Frederic Branczyk <fbranczyk@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.
Let's do follow ups, if necessary, in new PRs! :)
Thaaaaaaaaanks soo much for this! 🎉
This patch introduces a few changes:
.withServiceMonitor
.@metalmatze @squat @bwplotka @kakkoyun
Having reviewed this myself a few times, I recommend reading the generated manifests first, and then reviewing the new jsonnet files in their entity instead of comparing to the previous one.