-
Notifications
You must be signed in to change notification settings - Fork 64
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
enhancement: read grok aliases from file #194
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.
Added a few comments, overall it seems to be reasonable though. There was a large refactoring recently that moved most of the files from lib/*
to src/*
. The merge conflicts you are seeing are probably from that.
lib/compiler/src/function.rs
Outdated
@@ -519,18 +520,22 @@ pub enum Error { | |||
|
|||
#[error(r#"mutation of read-only value"#)] | |||
ReadOnlyMutation { context: String }, | |||
|
|||
#[error(r#"invalid alias source"#)] | |||
InvalidAliasSource { path: PathBuf }, |
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.
function specific errors should not go here. I would suggest just re-using the existing InvalidArgument
error above.
lib/stdlib/src/parse_groks.rs
Outdated
expr, | ||
})? | ||
.try_bytes_utf8_lossy() | ||
.expect("filename not bytes") |
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.
This should not panic, since nothing is making sure the inputs are strings here. The argument type is ARRAY
, but the elements can be any type. This should check that all arguments are strings, and return a InvalidArgument
if it's not.
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 realize you probably just copied this from aliases
above, but that is also a bug (that I just opened).
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.
OK. Will change.
lib/stdlib/src/parse_groks.rs
Outdated
"_status": "%{POSINT:status}", | ||
"_message": "%{GREEDYDATA:message}" | ||
}), | ||
alias_sources: Value::Array(vec![]), |
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 would like to see a test with a least 1 real alias file loaded. If it's easier, you can use the more general VRL testing framework at lib/tests/tests/functions/...
and include a file containing an alias.
90923b7
to
9e6d428
Compare
@fuchsnj I tackled the other alias thing as well, can you check if this is what you expect to happen? |
Yes, this looks great, thanks! |
Looks good overall. Please add a line to |
I was on holidays, will check asap. |
I think I fixed the wasm thing, let me know if anything else is needed. Thx! |
Hi @itkovian, there are some test failures. You can iterate on these locally by running |
Tests fail for the main branch as well?
|
Hi @itkovian, checks are passing on the current HEAD (ad17a96). The failure you posted above seems like an environment setup issue. Two commands to try:
If this is blocking you, you can just pick and choose what to run:
|
Hi @itkovian, just following up on this PR. Are you still interested in driving this to completion? |
@pront Yes, please :) Wasm build/tests should be fixed. |
CHANGELOG.md
Outdated
@@ -47,6 +47,7 @@ | |||
- fixed type definitions for side-effects inside of queries (https://github.com/vectordotdev/vrl/pull/258) | |||
- replaced `Program::final_type_state` with `Program::final_type_info` to give access to the type definitions of both the target and program result (https://github.com/vectordotdev/vrl/pull/262) | |||
- added `from_unix_timestamp` vrl function (https://github.com/vectordotdev/vrl/pull/277) | |||
- added the `alias_sources` parameter for `parse_groks` to read sources from files |
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.
Please move this line under "unreleased".
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.
Thank you @itkovian. Two things remain before we can merge this PR:
- We are missing a real test.
- I left a question about security.
Feel free to ignore the nits.
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.
Thank you for this contribution @itkovian!
Left one comment and also some CI checks are failing (running cargo fmt
should fix them).
Just realizing we never documented this addition in the Vector documentation. If you feel inspired, would you mind doing that @itkovian ? See vectordotdev/vector#19081 for an example. |
@jszwedko Sure. |
This PR aims to address (in part) #89, allowing GROK aliases to be defined in one or more files. I am not sure
(recreated from vectordotdev/vector#14914)