Fixes #8228: Add constraints to ncf parameters #350

VinceMacBuche · 2016-05-08T21:27:56Z

https://www.rudder-project.org/redmine/issues/8228

jooooooon · 2016-05-10T12:16:32Z

tools/ncf.py

@@ -16,6 +16,8 @@
 import sys
 import os
 import codecs
+import ncf_constraints
+from pprint import pprint


You can remove this now, I guess it was just used for debugging

I never remove pprint, you always need it.
Having to add it each time you debug and removing it just after is very tiring. Moreover it costs almost nothing.

I clearly agree with peck, we always need it

OK, fine. It's nice to answer comments though, even if you don't agree with them :)

VinceMacBuche · 2016-05-11T16:19:07Z

PR updated

VinceMacBuche · 2016-05-11T16:20:52Z

As a reminder:
Still need to add comments.
Check default cases are still treated (maybe define default constraint: allow_empty : false, allow_witthepace: false)

jooooooon · 2016-05-11T23:30:59Z

tools/ncf_constraints.py

+    "list" : {
+        "check" : from_list
+      , "type"  : list
+    }


Re-reading the email discussion we had about specs makes me notice the name you've used here: "list". I don't think this is a good name, because it is not clear what "constraint" "list" is imposing.

I prefer "enum" (which, to me at least, makes it clear that this constraint in an enumeration of possible values), but I'm open to other terms. Maybe "values"?

Thoughts?

if it's an enumeration, in the web world we use "select", which is fairly easily understood.

I like the 'select' ! second choice would be 'values'. I don't like 'enum', and agree that list is not a good choice

'select' works for me. It matches nicely with Rudder's Techniques metadata.xml terminology, too.

VinceMacBuche · 2016-05-23T09:43:43Z

PR updated

peckpeck · 2016-05-30T14:11:08Z

There should be an issue for documenting the constraint syntax

peckpeck · 2016-05-30T14:11:15Z

This PR needs a rebase

peckpeck · 2016-05-30T14:22:13Z

tools/ncf_constraints.py

+
+def match_regex(parameter, regex):
+  match = re.search(regex, parameter)
+  return match is not None


re.S and re.M should be considered
It means that a parameter should be considered as a whole and we are matching it as is, not with separate lines. A new line can still be matched with \n, which is expected. And we can match things that couldn't be matched without those flags.

Example:

the regex r"^A.\nB.$" matches a line starting with A followed by a line starting with B. This is not possible without re.M

with re.S r"A.B." could match 2 lines with A in the first one and B in the second one. This may be counter intuitive.

So I'd say, use re.MULTILINE only.

Anyway, whatever the final behavior, the multiline case must be carefully documented.

We should use re.UNICODE too, because all our strings are unicode and i would expect '\w' to match 'é' in the general case.

I don't think we should use re.M

I think we want to check the whole value with our regex, so with you first case you would match
A
A
B

If you use re.M and would not match with other cases, and i think we don't want to match

You're right, I mixed up the definition of re.M, it is the opposite.

peckpeck · 2016-05-30T14:30:17Z

Everything else is ok for me

VinceMacBuche · 2016-06-08T16:36:42Z

@peckpeck I made a separate commit so you can look on the changes
edit: typo, s/comment/commit...

peckpeck · 2016-06-13T12:44:18Z

tools/ncf_constraints.py


 def not_match_regex(parameter_value, regex):
-  return not match_regex(parameter_value, regex)
+  result = match_regex(parameter_value, regex) is not None


There should be a re.U here

It is changed in next commit :)

and match_regexp is not re.match !

VinceMacBuche · 2016-06-13T14:00:56Z

PR updated

VinceMacBuche · 2016-06-13T14:14:13Z

PR updated

peckpeck · 2016-06-13T14:26:49Z

tools/ncf_constraints.py

@@ -114,11 +114,8 @@ def check_parameter(parameter_value, parameter_constraints):
  constraint_set = constraints

  # Check that our value contains a variable or not
-  value_without_variables = re.sub(r'[\$@][\{\(][a-zA-Z0-9\[\]_.-]+[\}\)]', "", parameter_value)
-  if parameter_value != value_without_variables:
+  if re.search(r'[$@][{(][$@{(a-zA-Z0-9[\]_.-]+[})]', parameter_value) is not None :


Just for information, ][ instead of [] works too

VinceMacBuche · 2016-06-13T14:33:47Z

PR updated

Normation-Quality-Assistant · 2016-06-13T14:35:20Z

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge #350
-- Your faithful QA

jooooooon reviewed May 10, 2016
View reviewed changes

VinceMacBuche force-pushed the ust_8228/add_constraints_to_ncf_parameters branch from 956884c to 76b1c25 Compare May 11, 2016 16:19

jooooooon reviewed May 11, 2016
View reviewed changes

VinceMacBuche force-pushed the ust_8228/add_constraints_to_ncf_parameters branch from bb99c43 to f4d062d Compare May 23, 2016 09:43

peckpeck reviewed May 30, 2016
View reviewed changes

VinceMacBuche force-pushed the ust_8228/add_constraints_to_ncf_parameters branch 2 times, most recently from c006e07 to 2491551 Compare June 10, 2016 09:59

peckpeck reviewed Jun 13, 2016
View reviewed changes

VinceMacBuche force-pushed the ust_8228/add_constraints_to_ncf_parameters branch from 681c612 to 7efc3e6 Compare June 13, 2016 14:00

VinceMacBuche force-pushed the ust_8228/add_constraints_to_ncf_parameters branch from 7efc3e6 to 5abd7cb Compare June 13, 2016 14:14

peckpeck reviewed Jun 13, 2016
View reviewed changes

Fixes #8228: Add constraint to parameters

a808aa9

VinceMacBuche force-pushed the ust_8228/add_constraints_to_ncf_parameters branch from 5abd7cb to a808aa9 Compare June 13, 2016 14:33

VinceMacBuche added the Ready for merge label Jun 13, 2016

Normation-Quality-Assistant added the qa: Can't merge label Jun 13, 2016

VinceMacBuche merged commit a808aa9 into Normation:v0.x Jun 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #8228: Add constraints to ncf parameters #350

Fixes #8228: Add constraints to ncf parameters #350

VinceMacBuche commented May 8, 2016

jooooooon May 10, 2016

jooooooon May 11, 2016

peckpeck May 12, 2016

VinceMacBuche May 12, 2016

jooooooon May 12, 2016

VinceMacBuche commented May 11, 2016

VinceMacBuche commented May 11, 2016

jooooooon May 11, 2016

ncharles May 12, 2016

VinceMacBuche May 12, 2016

jooooooon May 12, 2016

VinceMacBuche commented May 23, 2016

peckpeck commented May 30, 2016

peckpeck commented May 30, 2016

peckpeck May 30, 2016

peckpeck May 30, 2016

VinceMacBuche Jun 8, 2016

peckpeck Jun 8, 2016

peckpeck commented May 30, 2016

VinceMacBuche commented Jun 8, 2016 •

edited

Loading

peckpeck Jun 13, 2016

VinceMacBuche Jun 13, 2016

VinceMacBuche Jun 13, 2016

VinceMacBuche commented Jun 13, 2016

VinceMacBuche commented Jun 13, 2016

peckpeck Jun 13, 2016

VinceMacBuche commented Jun 13, 2016

Normation-Quality-Assistant commented Jun 13, 2016

Fixes #8228: Add constraints to ncf parameters #350

Fixes #8228: Add constraints to ncf parameters #350

Conversation

VinceMacBuche commented May 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinceMacBuche commented May 11, 2016

VinceMacBuche commented May 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinceMacBuche commented May 23, 2016

peckpeck commented May 30, 2016

peckpeck commented May 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peckpeck commented May 30, 2016

VinceMacBuche commented Jun 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinceMacBuche commented Jun 13, 2016

VinceMacBuche commented Jun 13, 2016

Choose a reason for hiding this comment

VinceMacBuche commented Jun 13, 2016

Normation-Quality-Assistant commented Jun 13, 2016

VinceMacBuche commented Jun 8, 2016 •

edited

Loading