[filebeat][gcs] - Refactor & cleanup with updates to some default values and docs #41834
Conversation
… changes in default values
botelastic
bot
added
the
needs_team
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
|
mergify
bot
added
the
backport-8.x
ShourieG
added
the
Team:Security-Service Integrations
botelastic
bot
removed
the
needs_team
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
|
/test |
| The input can be configured to work with and without polling, though currently, if polling is disabled it will only | ||
| perform a one time passthrough, list the file contents and end the process. Polling is generally recommented for most cases | ||
| even though it can get expensive with dealing with a very large number of files. | ||
| The input can be configured to work with and without polling, though if polling is disabled, it will only perform a one time passthrough, list the file contents and end the process. |
There was a problem hiding this comment.
"passthrough" is not right here. See here.
x-pack/filebeat/input/gcs/config.go
Outdated
There was a problem hiding this comment.
The semantics of omitempty don't make sense with this change. Actually, they don't make sense without this change (omitempty is always a serialisation option), and the string doesn't exist in go-ucfg. Why are they here?
The semantics change though even without this; for example, if poll is not set in the previous implementation the value here is whatever default is in place, but now an absent setting will be treated as false.
There was a problem hiding this comment.
@efd6, I've removed the omitempty tags from most places except one where it makes sense.
As for the default values, the defaultConfig() needed to be called before the unpack, which has been added. This will provide a baseline from things getting misconfigured.
There was a problem hiding this comment.
OK. Can you fix the comment for tryOverrideOrDefault? It refers to absence in the global config which is no longer possible since none of the fields are pointer values anymore.
I'm also wondering about why TimeStampEpoch is a pointer. I imagine that this is to check that it has been set. This is checked for being between the unix epoch (0) and a point far in the future. Is the unix epoch not a good default?
There was a problem hiding this comment.
I think the initial philosophy behind timestamp epoch was that any value would mean that it had been set and that it would be up to the user to control the value.
There was a problem hiding this comment.
How does that differ from, e.g. poll?
There was a problem hiding this comment.
Poll is now always set to true by default and we treat it as some value exists. So if poll is not set we would still set it. But in case of timestamp epoch if no value is set we won't trigger the timestamp check filter
There was a problem hiding this comment.
I've updated the comments
…ues and docs (#41834) (#41860) (cherry picked from commit 01cc134) Co-authored-by: ShourieG <[email protected]>
…ates to some default values and docs (#41986) * [filebeat][gcs] - Refactor & cleanup with updates to some default values and docs (#41834) (cherry picked from commit 01cc134) * Update CHANGELOG.next.asciidoc --------- Co-authored-by: ShourieG <[email protected]>
Type of change
Proposed commit message
This PR is a successor to an older closed PR. The older PR has been split into two parts, this being the one containing the necessary refactor and updates to default values and docs.
Default values have been updated to be more in line with standard mode of operations.
Checklist
- [] I have made corresponding change to the default configuration filesCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Disruptive User Impact
Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs