Skip to content
This repository has been archived by the owner on Oct 17, 2018. It is now read-only.

Use kv configuration from m3cluster #87

Merged
merged 4 commits into from
Sep 29, 2017
Merged

Use kv configuration from m3cluster #87

merged 4 commits into from
Sep 29, 2017

Conversation

cw9
Copy link
Contributor

@cw9 cw9 commented Sep 29, 2017

@cw9 cw9 changed the title Pass environemnt to kv store Use kv configuration from m3cluster Sep 29, 2017
Copy link
Contributor

@jeromefroe jeromefroe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@coveralls
Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage increased (+0.02%) to 88.688% when pulling 0457304 on chao-up-m3c into 4418449 on master.

@@ -85,7 +86,7 @@ func (cfg *Configuration) NewOptions(
instrumentOpts instrument.Options,
) (Options, error) {
// Configure rules kv store.
rulesStore, err := kvCluster.Store(cfg.RulesKVNamespace)
rulesStore, err := kvCluster.Store(cfg.RulesKVConfig.NewOptions())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to validate the options here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The opts.Validate() is called in the Store()

@coveralls
Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage decreased (-0.04%) to 88.627% when pulling 0457304 on chao-up-m3c into 4418449 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 88.627% when pulling 0457304 on chao-up-m3c into 4418449 on master.

@cw9 cw9 merged commit 82d18cd into master Sep 29, 2017
@cw9 cw9 deleted the chao-up-m3c branch September 29, 2017 21:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants