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

Remove deprecated Init(), pass instance.Settings around. #10721

Merged
merged 6 commits into from
Feb 14, 2019

Conversation

simitt
Copy link
Contributor

@simitt simitt commented Feb 13, 2019

Currently following deprecated methods do not take instance.Settings as argument GenRootCmd, GenRootCmdWithRunFlags and GenRootCmdWithIndexPrefixWithRunFlags. Internally some commands also don't take the instance.Settings as params (genSetupCmd, genKeystoreCmd, ..). Since the introduction of settings.ILM and settings.IndexManagement (https://github.com/elastic/beats/blob/master/libbeat/cmd/instance/settings.go#L41) this can lead to errors, as the defined Index and ILM Supporters might not be respected.

This PR intends to

  • remove all deprecated exported commands generating the root command.
  • ensure that all unexported methods regarding commands take the instance.Setting as parameter so all configurations can be respected.

fixes #10720

Ensure settings from rootCmd are respected also when calling `export`
and `setup`.
fixes elastic#10720
@simitt simitt requested a review from a team as a code owner February 13, 2019 09:46
libbeat/cmd/test/output.go Show resolved Hide resolved
libbeat/cmd/test/config.go Show resolved Hide resolved
libbeat/cmd/export/template.go Show resolved Hide resolved
libbeat/mock/mockbeat.go Show resolved Hide resolved
libbeat/libbeat.go Show resolved Hide resolved
@simitt
Copy link
Contributor Author

simitt commented Feb 14, 2019

Failing tests are unrelated afaik.

@kvch
Copy link
Contributor

kvch commented Feb 14, 2019

libbeat/cmd/version.go does not use instance.Settings right now. Is that intentional?
I mean I see that it is not consistent over the commands at all. Shouldn't we use a settings instance in gen*Cmd functions?

EDIT: I also found x-pack/libbeat/cmd/enroll.go and x-pack/functionbeat/cmd/provider_cmd.go under x-pack.

@kvch kvch self-requested a review February 14, 2019 11:28
@@ -681,6 +681,7 @@ func (b *Beat) loadDashboards(ctx context.Context, force bool) error {
}

if b.Config.Dashboards.Enabled() {

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this line.

@simitt
Copy link
Contributor Author

simitt commented Feb 14, 2019

While the versionCmd and the completionCmd do not need the settings info for their use case, I think it would be cleaner to also pass in the settings object to be aligned. Will change it, thanks for pointing out @kvch .

@kvch
Copy link
Contributor

kvch commented Feb 14, 2019

Thank you!

@simitt
Copy link
Contributor Author

simitt commented Feb 14, 2019

jenkins, retest this please

@simitt simitt added the needs_backport PR is waiting to be backported to other branches. label Feb 14, 2019
@simitt simitt merged commit d83bb4d into elastic:master Feb 14, 2019
simitt added a commit to simitt/beats that referenced this pull request Feb 15, 2019
* Remove deprecated Init(), pass instance.Settings around.

Ensure settings from rootCmd are respected also when calling `export`
and `setup`.

fixes elastic#10720
simitt added a commit that referenced this pull request Feb 15, 2019
… (#10759)

Remove deprecated Init(), pass instance.Settings around. (#10721)
Ensure settings from rootCmd are respected also when calling `export`
and `setup`.

fixes #10720
@simitt simitt deleted the cmds-use-settings branch May 6, 2019 15:30
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
* Remove deprecated Init(), pass instance.Settings around.

Ensure settings from rootCmd are respected also when calling `export`
and `setup`.

fixes elastic#10720
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs_backport PR is waiting to be backported to other branches. review v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libbeat cmds do not respect instance.Settings
3 participants