-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fixes #11913: no reporting if value to report contains a variable #674
Fixes #11913: no reporting if value to report contains a variable #674
Conversation
tools/ncf.py
Outdated
class_parameter_id = method_info["class_parameter_id"] | ||
class_parameter_name = method_info["parameter"][class_parameter_id]["name"] | ||
class_parameter_value = method_call["args"][class_parameter_id] | ||
content.append(' "'+promiser+'_context" usebundle => current_technique_report_info("'+method_info["name"]+'", "'+class_parameter_name+'", "'+class_parameter_value+'");') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it properly escaped ? what if it component key contains " or ' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't match current_technique_report_info call, the first parameter is technique_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ncharles ! this needs escaping you are right :( :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @peckpeck it should be the technique name not the method name ...
Commit modified |
bd4d6d8
to
7e370c1
Compare
Commit modified |
7e370c1
to
c369e7d
Compare
Commit modified |
c369e7d
to
4649b16
Compare
PR rebased |
4649b16
to
d37ea25
Compare
Commit modified |
d37ea25
to
d0cff26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a question, and a request for indentation :)
@@ -550,6 +550,7 @@ def generate_technique_content(technique, methods): | |||
for method_call in technique["method_calls"]: | |||
method_name = method_call["method_name"] | |||
method_info = methods[method_name] | |||
# regex to match quote characters not preceded by a backslash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems to me it does a little more than backslash, or else the regexes lines 581-583 are not complete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's exactly that, the regex here in 554 is using a negative lookbehind:
(?<!text)
to match quotes not preceded by a backslash
then 581-583 use the regexp defined in 554 to replace the unescaped quotes by an escaped quote
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad ! Thank you for the clarification
tree/20_cfe_basics/log_rudder.cf
Outdated
"pass3" expression => "pass2"; | ||
"pass2" expression => "pass1"; | ||
"pass1" expression => "any"; | ||
"c_class_prefix" string => canonify("${old_class_prefix}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add 2 spaces of indentation ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's correctly idented, there is no class defined
vars: "c_class_prefix" ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or do indent 6 spaces even if there is no class ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, even if no class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK from what i read elsewhere we do likewise !! thanks @ncharles !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i reindented with 6 spaces (and the report below is also now indented with 6 spaces)
Commit modified |
d0cff26
to
f61dd73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me !
OK, merging this PR |
https://www.rudder-project.org/redmine/issues/11913