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

[bwplotka]

Hi :wave:

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 :smiling_imp:

[PhilipGough]

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

[bwplotka]

Sure @PhilipGough :heart:

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 https://github.com/thanos-io/thanos/pull/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[bot]]

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] [e-mail] 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
https://github.com/thanos-io/thanos/issues/2248#issuecomment-612629554,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABVA3O5ESEKKGKOE3NGTA3LRMHJQTANCNFSM4LFA7G2Q
.

[stale[bot]]

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[bot]]

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.

[prmsrswt]

Still valid

[stale[bot]]

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[bot]]

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

[stale[bot]]

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[bot]]

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