cmd/thanos: Refactor run*(...) functions.

[bwplotka]

Hi 👋

Right now all our components can be run as a single commands. We do this via those ugly run functions with a sometimes a large number of arguments (one per each flag). e.g. https://github.com/thanos-io/thanos/blob/master/cmd/thanos/query.go#L132 It would be nice to fix this while preserving flags. The short fix would be to create config structs for each command that will be populated by flags, and then pass it to run functions?

AC:

• No more functions with 20+ arguments in Thanos.

[bwplotka]

Wonder if this helps https://github.com/peterbourgon/ff

[bwplotka]

cc@s-urbaniak as you were complaining about this 😈

[philipgough]

@bwplotka I don't mind taking a stab at this if thats ok

[bwplotka]

Sure @philipgough ❤️

Let us know here @philipgough what would be idea of end look .e.g. how you plan to achieve this. (:

[philipgough]

@bwplotka I did some work on the sidecar component only to get some feedback. See #2267

Basically, the idea I had was to decompose into smaller structs with related config and compose the config for the component from these.

Config structs are built by passing all flags into a func. The benefit here is that we wouldn't need to duplicate this info in each component, the downside (I haven't checked if its required) is the lack of ability to customise defaults in each component. Should the defaults always be the same?

I took a look at the package you linked and while it would probably work, would require a change from kingpin lib to flag package in stdlib. Maybe this is worth considering but I would have thought unlikely?

I didn't see that the kingpin library had a way to populate struct fields directly during parse unfortunately

[stale]

This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions.

[bwplotka]

Almost done (:

…

On Sun, 12 Apr 2020 at 16:00, stale[bot] ***@***.***> wrote: This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2248 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O5ESEKKGKOE3NGTA3LRMHJQTANCNFSM4LFA7G2Q> .

[stale]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[soniasingla]

@bwplotka i would like to play with this bug 😺

[philipgough]

@soniasingla I have some completed already but feel free to take any of the others. I am working through query at the moment, compact and sidecar are done.

[stale]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[onprem]

Still valid

[stale]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Closing for now as promised, let us know if you need this to be reopened! 🤗

[MalloZup]

ack 👾

[bwplotka]

Maybe @idoqo, you want help here? Sounds like it's defined and we need this badly 🤗

[bwplotka]

I think querier, is almost done by @MalloZup , but let's look on other components if we can do the similar change as #3771 (review)

[idoqo]

While implementing the config struct for ruler, I noticed some advantage in the current approach (using the run* methods) and I'm not sure how to apply them to the "config structs" method yet.

Using the run* methods as in

thanos/cmd/thanos/rule.go

Line 291 in 04ad33d

func runRule(

), we can figure all the flags that are applicable to a component.

Abstracting those parameters into config structs could take that away for instance: shipperConfig has four fields, but ruler only uses allowOutOfOrderUpload (i.e., shipper.allow-out-of-order-uploads). I feel like passing shipperConfig as a struct field for any config will imply that it accepts all four flags which is wrong.

[someshkoli]

Did similar refactoring for store 🤔 #4061

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Closing for now as promised, let us know if you need this to be reopened! 🤗

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[metonymic-smokey]

Are there any commands left to complete?
Wanted to take a stab at it since it's a good first issue

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[yeya24]

Still valid

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Closing for now as promised, let us know if you need this to be reopened! 🤗