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.

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

(#1952) Fix fact filter parsing #1954

Merged
merged 2 commits into from
Jan 10, 2023
Merged

Conversation

ploubser
Copy link
Contributor

@ploubser ploubser commented Jan 9, 2023

With the addition of GJson to describe fact values the old fact parser is no longer able to correctly identify facts vs operators in examples like 'storage.#(name=="vda").size'.

Here we rewrite the ParseFactFilterString function to no longer used complicated regular expressions. We will now correctly identify GJson facts and parse the filters correctly.

In addition we're adding a wider range of tests for ParseFactFilterString so that we're covered if we were to add more fact filter functionality in the future.

Copy link
Member

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking pretty good, minor comments

@ploubser ploubser force-pushed the issue_1952 branch 2 times, most recently from 0e09310 to 4e44c34 Compare January 10, 2023 09:51
@ploubser ploubser requested a review from ripienaar January 10, 2023 09:59
With the addition of GJson to describe fact values the old fact parser is no
longer able to correctly identify facts vs operators in examples like
`'storage.#(name=="vda").size'`.

Here we rewrite the ParseFactFilterString function to no longer used complicated
regular expressions. We will now correctly identify GJson facts and parse the
filters correctly.

In addition we're adding a wider range of tests for ParseFactFilterString so
that we're covered if we were to add more fact filter functionality in the
future.
Copy link
Member

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ripienaar ripienaar merged commit b657d53 into choria-io:main Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants