Skip to content
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

[m3query] Allow for optional override config file. #1934

Merged
merged 3 commits into from Sep 11, 2019

Conversation

@pavelnikolov
Copy link
Contributor

commented Sep 9, 2019

What this PR does / why we need it:

Fixes #1921

TL;DR Some of the config is static and can be stored in git. However, there are parts of the config that are generated dynamically like, for example, the RPC remote addresses, which are generated using service-discovery in our case. This PR allows for multiple config files.

Special notes for your reviewer:

This is a draft proposal for additional configuration of m3query. This is a WIP and chances are that this will break unit tests of components. Before I invest more time in this I'd like to know if you agree with this approach. Ideally, I'd prefer allowing for multiple -f arguments, like this:

./m3query -f common.yaml -f production.yaml -f remotes.yaml

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

- allow multiple config files

Does this PR require updating code package or user-facing documentation?:

YES

- allow override config file
@CLAassistant

This comment has been minimized.

Copy link

commented Sep 9, 2019

CLA assistant check
All committers have signed the CLA.

@pavelnikolov pavelnikolov force-pushed the pavelnikolov:allow-multiple-config-files branch 2 times, most recently from 244a146 to e8f1ffc Sep 9, 2019

@martin-mao martin-mao requested review from arnikola and robskillington Sep 9, 2019

@pavelnikolov pavelnikolov force-pushed the pavelnikolov:allow-multiple-config-files branch from e8f1ffc to 0839f06 Sep 10, 2019

@arnikola
Copy link
Collaborator

left a comment

Looks good in general, a couple of nits mostly about naming of the xconfig utility

src/x/config/flag_test.go Show resolved Hide resolved
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

package config_test

This comment has been minimized.

Copy link
@arnikola

arnikola Sep 10, 2019

Collaborator

meganit: we typically don't use the _test package name convention

This comment has been minimized.

Copy link
@pavelnikolov

pavelnikolov Sep 11, 2019

Author Contributor

It was intentional because I am not using any private members of the package. If anything internal changes, the tests remain the same - only test the public API of a package. There should be very few reasons not to use _test, IMHO.

src/x/config/flag_test.go Show resolved Hide resolved
// Then it can be invoked like this:
// ./app -f file1.yaml -f file2.yaml -f valueN.yaml
// Finally, when the flags are parsed, the variable contains all the values.
type StringSlice []string

This comment has been minimized.

Copy link
@arnikola

arnikola Sep 10, 2019

Collaborator

Can we make this something more contextual, like FlagSlice? StringSlice is pretty generic and I think introducing flag-specific context is fine since this is built to implement flag.Value

Maybe something like MultiFlagAccumulator or something to convey that it accumulates duplicate values?


import "fmt"

// StringSlice represents a slice of strings. When used as a flag variable,

This comment has been minimized.

Copy link
@arnikola

arnikola Sep 10, 2019

Collaborator

nit: Mind adding var _ flag.Value = &StringSlice{} for a little extra context?


func main() {
flag.Var(&configFiles, "f", "configuration file(s)")

This comment has been minimized.

Copy link
@arnikola

arnikola Sep 10, 2019

Collaborator

What do you think about pulling all this configFiles stuff into xconfig.StringSlice, something like

xconfig.ParseConfigFilenames(flag string, description string) ([]string, bool)

That could handle the validation and make it a little easier to use? Don't mind either option much though, so up to you here 👍

This comment has been minimized.

Copy link
@pavelnikolov

pavelnikolov Sep 11, 2019

Author Contributor

I'd personally leave it like this for now.

@arnikola arnikola added the ci label Sep 10, 2019

@schallert schallert removed the ci label Sep 10, 2019

@arnikola arnikola added the ci label Sep 10, 2019

@schallert schallert removed the ci label Sep 10, 2019

@arnikola arnikola added the ci label Sep 10, 2019

@schallert schallert removed the ci label Sep 10, 2019

@arnikola arnikola added the ci label Sep 10, 2019

@schallert schallert removed the ci label Sep 10, 2019

@pavelnikolov pavelnikolov force-pushed the pavelnikolov:allow-multiple-config-files branch from 1b59798 to 3e9a3ed Sep 11, 2019

@pavelnikolov pavelnikolov requested a review from arnikola Sep 11, 2019

@arnikola arnikola added the ci label Sep 11, 2019

@schallert schallert removed the ci label Sep 11, 2019

@arnikola arnikola merged commit db4dc03 into m3db:master Sep 11, 2019

4 checks passed

buildkite/m3/pr Build #632 passed (13 minutes, 23 seconds)
Details
ci-gate Pull Request accepted for CI
license/cla Contributor License Agreement is signed.
Details
netlify/m3db/deploy-preview Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.