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

added brakets to three macros to make them less ambiguous #1373

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

ldegio
Copy link
Contributor

@ldegio ldegio commented Aug 28, 2020

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

If contributing rules or changes to rules, please make sure to also uncomment one of the following line:

/kind rule-update

/kind rule-create

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area rules

/area tests

/area proposals

What this PR does / why we need it:

rules that mix and operators with or operators in a flat way (i.e. without brackets to explicitly separate them) have a couple of challenges:

  • they are ambiguous to read, because the falco filter interpreter doesn't currently implement operator precedence
  • they are not parsed correctly, because the filter interpreter sometimes is not capable of fully evaluating them

It turns out that there are only 3 macros in the whole ruleset that mix and and or operators without brakets. This PR modifies them by adding the required brakets.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

rule(macro inbound_outbound): add brackets to disambiguate operator precedence
rule(macro redis_writing_conf): add brackets to disambiguate operator precedence
rule(macro run_by_foreman): add brackets to disambiguate operator precedence

fntlnz
fntlnz previously approved these changes Aug 31, 2020
Copy link
Contributor

@fntlnz fntlnz left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@poiana
Copy link
Contributor

poiana commented Aug 31, 2020

LGTM label has been added.

Git tree hash: 86e77e76a89f7194b59d483965177c314a602031

leodido
leodido previously approved these changes Aug 31, 2020
Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

Thanks for this Loris! 🎉

I've updated the release-note block lines to make them match the guidelines.

Would you please just sign-off your commit and force push it? Thanks again 😃

@ldegio ldegio dismissed stale reviews from leodido and fntlnz via a457c92 August 31, 2020 15:06
@ldegio ldegio force-pushed the minor-rule-disambiguation branch from 4fe3399 to a457c92 Compare August 31, 2020 15:06
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

LGTM

@poiana
Copy link
Contributor

poiana commented Aug 31, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fntlnz, leodido, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit 5d71d70 into master Aug 31, 2020
@poiana poiana deleted the minor-rule-disambiguation branch August 31, 2020 16:02
@krisnova krisnova added this to the 0.26.0 milestone Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants