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

Enhance config.Namespace #4339

Merged
merged 1 commit into from
Jun 22, 2017
Merged

Enhance config.Namespace #4339

merged 1 commit into from
Jun 22, 2017

Conversation

urso
Copy link

@urso urso commented May 17, 2017

when unpacking into config.Namespace, namespaces can be disabled
via enabled: false now. For settings allowing exactly one configuration, this
can be used by users to enabled/disable namespace without having to comment
them out in the configuration file.

example usage:

type Config struct {
    Output common.ConfigNamespace
}

user can have at most one enabled:

output.namespace1:
  enabled: false

output.namespace2:
  enabled: true

@@ -327,33 +328,59 @@ func (f *flagOverwrite) Get() interface{} {
return f.value
}

func (ns *ConfigNamespace) Unpack(cfg *Config) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported method ConfigNamespace.Unpack should have comment or be unexported

@ruflin
Copy link
Contributor

ruflin commented May 18, 2017

I'm not 100% sure I fully understand what this feature is doing compare to before. What wasn't possible and what is possible now (or the other way around)?

// Validate checks at most one sub-namespace being set.
func (ns *ConfigNamespace) Validate() error {
if len(ns.C) > 1 {
return errors.New("more then one namespace configured")
}
return nil
}
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this code be removed?

Copy link
Author

Choose a reason for hiding this comment

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

ups, yes.

@urso
Copy link
Author

urso commented May 18, 2017

this was not possible before:

output.namespace1:
  enabled: false

output.namespace2:
  enabled: true

when unpacking this config, ConfigNamespace will raise an error, because it found namespace1 and namespace2 stored. This PR enables this use-case, as the new Unpack on ConfigNamespace ignores namespaces with enabled: false.

@urso urso force-pushed the enh/config-namespaces branch from 3304169 to 5491ad9 Compare May 18, 2017 13:18
func (ns *ConfigNamespace) Validate() error {
if len(ns.C) > 1 {
return errors.New("more then one namespace configured")
func (ns *ConfigNamespace) Unpack(cfg *Config) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add here some docs on what is happening if more then 1 namespace is enabled?

Copy link
Author

Choose a reason for hiding this comment

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

done

func (ns *ConfigNamespace) Validate() error {
if len(ns.C) > 1 {
return errors.New("more then one namespace configured")
func (ns *ConfigNamespace) Unpack(cfg *Config) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
exported method ConfigNamespace.Unpack should have comment or be unexported

@urso urso force-pushed the enh/config-namespaces branch from 5491ad9 to 8cc6a01 Compare June 9, 2017 14:42
@ruflin
Copy link
Contributor

ruflin commented Jun 12, 2017

@urso Build seems to be broken :-(

when unpacking into config.Namespace, namespaces can be disabled
via `enabled: false` now. For settings allowing exactly one configuration, this
can be used by users to enabled/disable namespace without having to comment
them out in the configuration file.

example usage:

```
type Config struct {
    Output common.ConfigNamespace
}
```

user can have at most one enabled:

```
output.namespace1:
  enabled: false

output.namespace2:
  enabled: true
```
@urso urso force-pushed the enh/config-namespaces branch from 8cc6a01 to 6167cc8 Compare June 19, 2017 15:57
@tsg tsg merged commit a0b4797 into elastic:master Jun 22, 2017
@urso urso deleted the enh/config-namespaces branch February 19, 2019 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants