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

Make default path.X available in config file #2042

Merged
merged 3 commits into from
Jul 19, 2016
Merged

Conversation

urso
Copy link

@urso urso commented Jul 15, 2016

No description provided.

@urso urso added in progress Pull request is currently in progress. discuss Issue needs further discussion. review labels Jul 15, 2016
@@ -209,7 +209,7 @@ filebeat.prospectors:

# Name of the registry file. If a relative path is used, it is considered relative to the
# data path.
#filebeat.registry_file: registry
#filebeat.registry_file: ${path.data}/registry
Copy link
Contributor

Choose a reason for hiding this comment

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

Writing filebeat.registry_file: ${path.data}/registry will be equivalent with writing filebeat.registry_file: registry, right?

I like the version with ${path.data} as it's more explicit about where the file is searched. But we'll need to explain in the docs that these variables are automatically set somehow.

Copy link
Author

Choose a reason for hiding this comment

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

yes, it's equivalent.

@tsg
Copy link
Contributor

tsg commented Jul 15, 2016

Code LGTM. There's a unit test failure at the moment.

Unfortunately I think we don't have system tests for these, so I guess we'll need to do some manual tests to check that:

@urso
Copy link
Author

urso commented Jul 15, 2016

uh, we're missing path.bin ?

@tsg
Copy link
Contributor

tsg commented Jul 17, 2016

@urso yeah, we don't have path.bin configurable since it is always the location of the binary. Do you need it for a config setting or is it just for consistency with ES?

@@ -238,6 +241,8 @@ func (b *Beat) configure() error {
return fmt.Errorf("error loading config file: %v", err)
}

// init paths config:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that comment belong on line 251?

Copy link
Author

Choose a reason for hiding this comment

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

line can be removed. comment left over from original failed (more complicated) approach.

@urso
Copy link
Author

urso commented Jul 18, 2016

@tsg

yeah, we don't have path.bin configurable since it is always the location of the binary. Do you need it for a config setting or is it just for consistency with ES?

Just for consistency. But as there is currently no usage for it, let's keep it out.

@urso urso force-pushed the enh/path-config branch from 2f0665b to 82f9556 Compare July 18, 2016 19:41
@urso urso removed the in progress Pull request is currently in progress. label Jul 19, 2016
@tsg
Copy link
Contributor

tsg commented Jul 19, 2016

Looks good on some brief manual testing. @urso happy to merge it, do you want to cleanup history or should I just squash it?

@tsg tsg merged commit 297ea34 into elastic:master Jul 19, 2016
@urso urso deleted the enh/path-config branch February 19, 2019 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issue needs further discussion. review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants