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 short description how rules can be re-ordered #402

Merged
merged 2 commits into from May 12, 2022

Conversation

zerwes
Copy link
Contributor

@zerwes zerwes commented May 12, 2022

While reading a forum post, I realized the was rules can be reordered is missing from the docs.
Here a short chapter is added after the Processing order as I was thinking this is the right place.

@fichtner
Copy link
Member

fichtner commented May 12, 2022

I'm not against this, but seeing that "ordering" has two meanings here on the page I think this will increase confusion. This one could be referenced as "Rule sequence" and trying to stick to "reordering" in the text? (It's tricky either way.)

@zerwes
Copy link
Contributor Author

zerwes commented May 12, 2022

You are right, this might lead to more confusion then helping ...
Sequence seems the right here esp as it is used in line 94 "Firewall rules are processed in sequence, first evaluating the Floating rules section ..."
Thank you!

Proposal:

diff --git a/source/manual/firewall.rst b/source/manual/firewall.rst
index 6789db1..62228fd 100644
--- a/source/manual/firewall.rst
+++ b/source/manual/firewall.rst
@@ -125,12 +125,12 @@ Our default deny rule uses this property for example (if no rule applies, drop t
     The interface should show all rules that are used, when in doubt, you can always inspect the raw output of the ruleset in :code:`/tmp/rules.debug`
 
 ....................
-Ordering rules
+Rule sequence
 ....................
 
-.. _Firewall_Rule_Ordering_Rules:
+.. _Firewall_Rule_Rule_Sequence:
 
-The order in which the rules are displayed and processed can be customized:
+The sequence in which the rules are displayed and processed can be customized:
 
 * Select one or more rules using the checkbox on the left side of the rule.
 * Use the arrow button in the action menu on the right side of a rule in order to move selected rules before the rule where the action button is pressed.

If this is OK, I can commit it ...

@fichtner
Copy link
Member

I'd argue the processing order section shouldn't use "sequence" then as well. Also "Firewall_Rule_Rule_Sequence" is odd :D

@zerwes
Copy link
Contributor Author

zerwes commented May 12, 2022

Also "Firewall_Rule_Rule_Sequence" is odd

:-) more then odd :-)

-.. _Firewall_Rule_Rule_Sequence:
+.. _Firewall_Rule_Sequence:

I'd argue the processing order section shouldn't use "sequence" then as well.

OK, semantics are sometimes difficult and complex, but I do not get the point here ...
There are sections and sections have a processing order, and per section the rules have a sequence defining the processing order within the section ... so from my point using sequence here seems OK, otherwise one had do distinguish between order of sections and order of rules per section ...
Maybe

diff --git a/source/manual/firewall.rst b/source/manual/firewall.rst
index 6789db1..cc9536f 100644
--- a/source/manual/firewall.rst
+++ b/source/manual/firewall.rst
@@ -91,7 +91,7 @@ Processing order
 
 .. _Firewall_Rule_Processing_Order:
 
-Firewall rules are processed in sequence, first evaluating the **Floating** rules section followed by all rules which
+Firewall rules are processed in sequence per section, first evaluating the **Floating** rules section followed by all rules which
 belong to **interface groups** and finally all **interface** rules.
 
 Internal (automatic) rules are usually registered first.

could make this clearer ...

Maybe a proofreading editorial section of native speaker without technical background should be hired from the community :-)

@fichtner
Copy link
Member

Fair enough... and now that we're referencing "per section" before we describe per section shouldn't your new paragraph go before the processing order? :)

@fichtner fichtner self-assigned this May 12, 2022
@zerwes
Copy link
Contributor Author

zerwes commented May 12, 2022

:) I think the sequence of the sections aka. paragraphs is processed in the right order this way :-) ... from the big picture to the detail per section ...

diff --git a/source/manual/firewall.rst b/source/manual/firewall.rst
index 6789db1..df1dc45 100644
--- a/source/manual/firewall.rst
+++ b/source/manual/firewall.rst
@@ -91,7 +91,7 @@ Processing order
 
 .. _Firewall_Rule_Processing_Order:
 
-Firewall rules are processed in sequence, first evaluating the **Floating** rules section followed by all rules which
+Firewall rules are processed in sequence per section, first evaluating the **Floating** rules section followed by all rules which
 belong to **interface groups** and finally all **interface** rules.
 
 Internal (automatic) rules are usually registered first.
@@ -125,12 +125,12 @@ Our default deny rule uses this property for example (if no rule applies, drop t
     The interface should show all rules that are used, when in doubt, you can always inspect the raw output of the ruleset in :code:`/tmp/rules.debug`
 
 ....................
-Ordering rules
+Rule sequence
 ....................
 
-.. _Firewall_Rule_Ordering_Rules:
+.. _Firewall_Rule_Sequence:
 
-The order in which the rules are displayed and processed can be customized:
+The sequence in which the rules are displayed and processed can be customized per section:
 
 * Select one or more rules using the checkbox on the left side of the rule.
 * Use the arrow button in the action menu on the right side of a rule in order to move selected rules before the rule where the action button is pressed.

should I commit?

@zerwes
Copy link
Contributor Author

zerwes commented May 12, 2022

just pushed the last commit
@fichtner feel free to make changes, I suspect you are far more skilled here

@fichtner fichtner merged commit cea489f into opnsense:master May 12, 2022
@fichtner
Copy link
Member

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants