Skip to content

Enhance config.Namespace#4339

Merged
tsg merged 1 commit intoelastic:masterfrom
urso:enh/config-namespaces
Jun 22, 2017
Merged

Enhance config.Namespace#4339
tsg merged 1 commit intoelastic:masterfrom
urso:enh/config-namespaces

Conversation

@urso
Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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)?

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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