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

Fix/relax types #45

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix/relax types #45

wants to merge 9 commits into from

Conversation

faxm0dem
Copy link
Member

We just converted our 632 patterns to the new v5 format.
We ran into a few incompatible changes, and this PR allows all our patterns to work out.

Basically we relax a few type constraints, which are a bit too tight compared to what's allowed in patterndb

smortex
smortex reviewed Aug 24, 2023
View reviewed changes
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

I added some in-line comments for minor things. The CI failures on Debian are due to syslog-ng/syslog-ng#4585 but there was no release since that so we can ignore them for now.

types/example.pp Show resolved Hide resolved
types/example.pp Outdated Show resolved Hide resolved
types/example.pp Outdated Show resolved Hide resolved
types/rule.pp Outdated Show resolved Hide resolved
smortex
smortex reviewed Aug 25, 2023
View reviewed changes
types/example.pp Outdated Show resolved Hide resolved
@faxm0dem
Copy link
Member Author

faxm0dem commented Aug 29, 2023 •
edited
Loading

Something very annoying also, is that value must be a String.
This is awkward:

test_values:
  i_am_a_number: '123'
  type: integer

This feels more right:

test_values:
  i_am_a_number: 123
  type: integer

Should we do Variant[String,Numeric] ?

@smortex
Copy link
Collaborator

smortex commented Aug 29, 2023

Should we do Variant[String,Numeric] ?

We can.

In this case we can probably also add Float to that list and assume the end-users will only pass literals and not compute the values (0.02 vs 0.1 * 0.2)?

smortex
smortex approved these changes Jan 13, 2025
View reviewed changes
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

This looks good. You suggested relaxing some data types which I am fine with if you want to do it.

You can also rebase this branch on top of master so that CI check it does not break something unexpectedly.

Anyway, LGTM!

@faxm0dem faxm0dem requested a review from a team as a code owner January 23, 2025 08:49
@faxm0dem
Copy link
Member Author

faxm0dem commented Jan 23, 2025 •
edited by smortex
Loading

I think we need to resolve #44 first as this one depends on (parts of) it

@smortex
Copy link
Collaborator

smortex commented Jan 23, 2025

I updated #44 and it is ready for another review/merging. I see you cherry-picked my commit in this branch, I guess that removing that commit and rebasing on top of master will resolve conflicts.

faxm0dem and others added 9 commits January 30, 2025 08:46
@faxm0dem
Allow example values to be empty
9e17329 
In patterndb, an empty string is a valid one
@faxm0dem
Allow a rule's ID to be empty
355430a 
Given `String[1] $id = $title` the rules will always have an id.
@faxm0dem
Allow program to be empty in an example
f93c206
@faxm0dem
Apparently, there's a null type
9cd0393 
```
Wrong match name='apache.query_param', value='', type='null', expected='', expected_type='string'
```
@faxm0dem @smortex
Make type explicitly optional
27e9323 
This makes the following invalid (yes, surprisingly it was valid before) :

```
{ 'type' => undef }
```

Co-authored-by: Romain Tartière <[email protected]>
@faxm0dem @smortex
Fix bareword
c87d6b5 
Co-authored-by: Romain Tartière <[email protected]>
@faxm0dem
Fix non-mandatory options
78eee85
@faxm0dem
Don't log output if merge/deploy/test works
ad14bfc
@faxm0dem
Allow for numbers in example value
51040de
@faxm0dem
Copy link
Member Author

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smortex smortex smortex approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@faxm0dem @smortex