[Auditbeat] Log error when Homebrew not installed #11667
Conversation
cwurm
added
review
needs_backport
|
Pinging @elastic/secops |
There was a problem hiding this comment.
This is changing the expectations for the MetricSetFactory interface. Up until now all implementations must return either a MetricSet or an error. This change allows a MetricSetFactory to return both values as nil. I'd rather not allow this since I have the expectation from a factory that it will either produce the value I'm asking for or I will get an error.
|
Ok, makes sense. I've changed to keeping the dataset running even if Homebrew is not installed. It will log an @andrewkroh Let me know what you think. |
There was a problem hiding this comment.
I think we should have some more unit tests for this metricset. For testing purposes if we can control the "homebrewCellarPath" then we can test the behavior when the path doesn't exist and we can test parsing by putting some fake homebrew data into a testdata/ directory for the package.
|
I've added a @andrewkroh Let me know what you think. |
| defer func() { | ||
| homebrewCellarPath = oldPath | ||
| }() | ||
| homebrewCellarPath = "../../../tests/files/homebrew/" |
There was a problem hiding this comment.
The conventional directory for this in Go would be testdata within this package’s directory. testdata is ignored by Go (I think this is documented in go help test).
There was a problem hiding this comment.
Hm, so tests/files/ already contains some other test files as well. Should they all move to a testdata?
There was a problem hiding this comment.
If they are used by the Go tests then I would move them into a testdata dir next to the tests that use them. It's not a hard rule, but it's a pretty common practice with this sort of data as also with golden-file tests.
There was a problem hiding this comment.
Ok, makes sense. For now - in this PR - I've moved the new homebrew directory. We should move the others later.
x-pack/auditbeat/module/system/package/package_homebrew_test.go
Outdated
Show resolved
Hide resolved
cwurm
force-pushed
the
package_no_homebrew
branch
from
a528cd6
to
dbe8c38
Compare
Change `package` dataset on macOS to logging an error when Homebrew is not installed instead of failing to start altogether. It will keep checking and start sending events as usual in case Homebrew is installed later. (cherry picked from commit bbe4991)
cwurm
added
v7.0.1
and removed
needs_backport
Change `package` dataset on macOS to logging an error when Homebrew is not installed instead of failing to start altogether. It will keep checking and start sending events as usual in case Homebrew is installed later. (cherry picked from commit bbe4991)
Change `package` dataset on macOS to logging an error when Homebrew is not installed instead of failing to start altogether. It will keep checking and start sending events as usual in case Homebrew is installed later. (cherry picked from commit bbe4991)
…stalled (#11949) * [Auditbeat] Log error when Homebrew not installed (#11667) Change `package` dataset on macOS to logging an error when Homebrew is not installed instead of failing to start altogether. It will keep checking and start sending events as usual in case Homebrew is installed later. (cherry picked from commit bbe4991)
At the moment, if Auditbeat is run with the default configuration (where the
packagedataset is enabled) on a Mac where Homebrew is not installed it will fail to start.This changes the behavior to only logging an error message:
2019-04-05T15:03:01.250+0100 ERROR [package] package/package.go:218 Failed to start package dataset on macOS. Error looking up /usr/local/Cellar - is Homebrew installed? Error: stat /usr/local/Cellar: no such file or directory