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

Look for the config files relative to the path.config flag #2245

Merged
merged 1 commit into from
Aug 13, 2016

Conversation

tsg
Copy link
Contributor

@tsg tsg commented Aug 11, 2016

This is a proposal fix for #2171. There's a bit of a chicken and egg
problem here, since defining the paths requires the configuration file
and the other way around. The implemented logic is to:

  • if the -path.config flag is used, look for the config file relative to
    it
  • if not, but -path.home flag is used, look for the config file relative
    to the home path
  • else, look relative to the current working directory

The last point is a BWC break, because in 1.x the configuration file was relative to the binary location.

@tsg tsg added in progress Pull request is currently in progress. review labels Aug 11, 2016
@tsg
Copy link
Contributor Author

tsg commented Aug 11, 2016

@urso, you should probably review this one.

@tsg tsg force-pushed the fix_cfgfile_default branch 3 times, most recently from a4d8136 to f71a4cb Compare August 12, 2016 06:59
@tsg tsg removed the in progress Pull request is currently in progress. label Aug 12, 2016
@@ -14,7 +14,7 @@ var (
// The default config cannot include the beat name as it is not initialized
// when this variable is created. See ChangeDefaultCfgfileFlag which should
// be called prior to flags.Parse().
configfiles = flagArgList("c", "beat.yml", "Configuration file `path`")
configfiles = flagArgList("c", "beat.yml", "Configuration file, relative to path.config")
Copy link
Member

Choose a reason for hiding this comment

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

An absolute path also works, correct?

@ruflin
Copy link
Member

ruflin commented Aug 12, 2016

LGTM. @urso Do you also want to have a look?

@urso
Copy link

urso commented Aug 12, 2016

LGTM

@tsg requires rebase.

This is a proposal fix for elastic#2171. There's a bit of a chicken and egg
problem here, since defining the paths requires the configuration file
and the other way around. The implemented logic is to:

* if the `-path.config` flag is used, look for the config file relative to
  it
* if not, but `-path.home` flag is used, look for the config file relative
  to the home path
* else, look into the binary location, mostly for backwards compatibility

I'm not sure we need the last point, we could leave it relative to the cwd,
like most tools would do it. But this requires a BWC break.
@tsg
Copy link
Contributor Author

tsg commented Aug 12, 2016

@urso rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants