Skip to content

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

Merged
simitt merged 6 commits intoelastic:masterfrom
simitt:cmds-use-settings
Feb 14, 2019
Merged

Remove deprecated Init(), pass instance.Settings around.#10721
simitt merged 6 commits intoelastic:masterfrom
simitt:cmds-use-settings

Conversation

@simitt
Copy link
Copy Markdown
Contributor

@simitt simitt commented Feb 13, 2019 •
edited
Loading

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

@simitt
Remove deprecated Init(), pass instance.Settings around.
5f8ee22 
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
houndci-bot
houndci-bot reviewed Feb 13, 2019
View reviewed changes
@simitt simitt mentioned this pull request Feb 13, 2019
Closed
houndci-bot
houndci-bot reviewed Feb 13, 2019
View reviewed changes
@simitt simitt requested a review from a team as a code owner February 13, 2019 11:35
@simitt
Copy link
Copy Markdown
Contributor Author

simitt commented Feb 14, 2019

Failing tests are unrelated afaik.

@kvch
Copy link
Copy Markdown
Contributor

kvch commented Feb 14, 2019 •
edited
Loading

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
kvch
kvch reviewed Feb 14, 2019
View reviewed changes
libbeat/cmd/instance/beat.go Outdated
}

if b.Config.Dashboards.Enabled() {

Copy link
Copy Markdown
Contributor

@kvch kvch Feb 14, 2019

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

kvch commented Feb 14, 2019

Thank you!

@simitt
Copy link
Copy Markdown
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
This was referenced Feb 14, 2019
Merged
Merged
simitt added a commit to simitt/beats that referenced this pull request Feb 15, 2019
@simitt
Remove deprecated Init(), pass instance.Settings around. (elastic#10721)
cd4feea 
* 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
@simitt
[7.0] Remove deprecated Init(), pass instance.Settings around. (#10721)…
e4d3db5 
… (#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
@simitt
Remove deprecated Init(), pass instance.Settings around. (elastic#10721)
18e6f2c 
* 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

Reviewers

2 more reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@kvch kvch kvch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug needs_backport PR is waiting to be backported to other branches. review v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

libbeat cmds do not respect instance.Settings

3 participants

@simitt @kvch @houndci-bot