-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add info to support cloud config #7813
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Left some comments
@@ -2,54 +2,119 @@ | |||
[id="running-{modulename}-modules"] | |||
=== Set up and run the module | |||
|
|||
IMPORTANT: If you’ve secured Elasticsearch and Kibana, you need to configure the | |||
`username` and `password` options in the Elasticsearch output before setting up | |||
// REVIEWERS: Should we include docker commands here, too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should, as it is not straightforward in the docker world to follow these interactive steps
at the command line when you run Filebeat. In a production environment, you'll | ||
probably want to use the configs in the `modules.d` directory instead. See | ||
<<configuration-filebeat-modules>> for more information. | ||
ownership or permissions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the note and the paragraph are talking about errors related to file ownership or permissions
. May this sound repetitive? I'm not sure here, so it's your call :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The info is in one note, but you're right...I don't have to repeast hte phraise "errors related to file ownership or permissions."
// what theses commands look like for deb/rpm/windows. I assume we want to | ||
// tell users to start the service, but I don't think options like -M are | ||
// allowed when you start as a service. Are they? Hoping someone with | ||
// deb/rpm/windows experience can help me get the syntax right here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no way to pass these variables to services, they would need to update the config file with them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@exekias Then I should probably add a note to indicate that. But I think users can still run the following commands, right?
deb and rpm:
filebeat -e -M "nginx.access.var.paths=[/var/log/nginx/access.log*]"
win:
PS > .\filebeat.exe -e -M "nginx.access.var.paths=[c:/programdata/nginx/logs/*access.log*]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those look correct 👍 but they are not useful to keep the service running, but for testing.
cc @dov0211 |
00f73ab
to
3343e6e
Compare
3343e6e
to
60fafce
Compare
I've added the fixes from the review. I did a lot of refactoring in the Filebeat docs to remove duplicated content. You can see what the changes will look like here: https://filebeatreference-6af8e.firebaseapp.com/ Now all the commands in the getting started and reference content should show platform-specific commands. @exekias Changes are ready for a final review. |
jenkins retest this please |
@karenzone FYI... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add info to support cloud config
* Add info to support cloud config
* Add info to support cloud config
* Add info to support cloud config
* Add info to support cloud config
* Add info to support cloud config
* Add info to support cloud config
* Add info to support cloud config
Summary of changes:
I'm marking this as "in progress" because I need some input from reviewers before this is ready to merge. Please see comments flagged for REVIEWERS.