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.

Sign up for GitHub

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

Jump to bottom

Vector shouldn't use the test VRL feature #17377

Closed
fuchsnj opened this issue May 12, 2023 · 2 comments · Fixed by #18740
Closed

Vector shouldn't use the test VRL feature #17377

fuchsnj opened this issue May 12, 2023 · 2 comments · Fixed by #18740
Labels
type: bug A code related bug.

Comments

@fuchsnj
Copy link
Member

fuchsnj commented May 12, 2023 •
edited
Loading

A note for the community

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Problem

Previously the vrl crate had an internal test feature that was un-intentionally enabled. This was caught and fixed in recent VRL refactoring. (VRL 0.4.0).

Vector was relying on this in a few places where is shouldn't be (all related to converting an f64 into a Value. That implementation is normally only available for tests, since the f64 needs to be checked for NaN first).
As a quick fix, to keep existing behavior, the test feature was explicitly enabled for VRL. This should be fixed and removed.

Version

0.29
@fuchsnj fuchsnj added the type: bug A code related bug. label May 12, 2023
@fuchsnj fuchsnj mentioned this issue May 12, 2023
Merged
fuchsnj added a commit that referenced this issue May 25, 2023
@fuchsnj
chore: update vrl to 0.4.0 (#17378)
426d660 
This upgrades VRL to `0.4.0`


Notable changes:
- This is the first crates.io release for VRL. It's no longer a git
dependency!
- All VRL macros are now exported at the root, which required some
import changes
- Previously the `vrl` crate had an internal `test` feature that was
un-intentionally enabled. This was caught and fixed in recent VRL
refactoring. Vector was relying on this in a few places where is
shouldn't be (all related to converting an `f64` into a `Value`. That
implementation is normally only available for tests, since the `f64`
needs to be checked for `NaN` first). As a quick fix, to keep existing
behavior, the `test` feature is now explicitly enabled for VRL. [An
issue](#17377) was created
to track this and remove it.
@dsmith3197
Copy link
Contributor

@pront please confirm that this is fixed.

@pront
Copy link
Member

pront commented Sep 28, 2023 •
edited
Loading

Vector still enables the test VRL feature. +1 on removing this. Also, worth checking if test_framework can also be removed.

@pront pront mentioned this issue Oct 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: bug A code related bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore(deps): clean up VRL crate features vectordotdev/vector
3 participants
@pront @fuchsnj @dsmith3197