Fixes #11142: filetemplate technique posthook #1177

Fdall · 2017-07-17T13:42:49Z

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

peckpeck · 2017-07-17T17:41:26Z

techniques/fileDistribution/fileTemplate/1.0/metadata.xml

+          </ITEM>
+
+          <CONSTRAINT>
+            <DEFAULT>false</DEFAULT>


I think the default should be true

peckpeck · 2017-07-17T17:41:50Z

techniques/fileDistribution/fileTemplate/1.0/metadata.xml

@@ -132,6 +132,24 @@
          </CONSTRAINT>
        </INPUT>

+        <SELECT1>
+          <NAME>FILE_TEMPLATE_PERSISTENT_POST_HOOK</NAME>
+          <DESCRIPTION>Persistent posthook command</DESCRIPTION>


Retry hook on error

peckpeck · 2017-07-17T18:40:30Z

techniques/fileDistribution/fileTemplate/1.0/fileTemplate.st

      "src_canon[${index}]" string => canonify("${relative_src}/${src[${index}]}");
      "dst_canon[${index}]" string => canonify("${dst[${index}]}");
      "temp_canon[${index}]" string => canonify("${temp}/${name[${index}]}");
      "temp_dir_canon" string => canonify("${temp}");
      "posthook_canon[${index}]" string => canonify("${posthook[${index}]}");

+      #Cancel persistent classes
+      pass3::


if posthook is ok, there should be no need for a pass3

still there

peckpeck · 2017-07-17T18:42:12Z

techniques/fileDistribution/fileTemplate/1.0/fileTemplate.st

-      "posthook_not_launch_${index}" expression =>"posthook_specified_${index}.!(file_modified_${index})";
+      "posthook_persistent_${index}" expression => strcmp("${persist[${index}]}", "true");
+
+      #Check if we are on the second posthook run or more


This comment is not true : we are saving the "posthook failed" information for next run

peckpeck · 2017-07-17T18:43:16Z

techniques/fileDistribution/fileTemplate/1.0/fileTemplate.st

+      #Check if we are on the second posthook run or more
+      "posthook_multi_runs_${index}" expression => "posthook_persistent_${index}.command_execution_${posthook_canon[${index}]}_failed",
+                                     scope => "namespace",
+                                     persistence => "10000";


Please comment on how long this number represents

peckpeck · 2017-07-17T18:44:13Z

techniques/fileDistribution/fileTemplate/1.0/fileTemplate.st

+      "posthook_persistent_${index}" expression => strcmp("${persist[${index}]}", "true");
+
+      #Check if we are on the second posthook run or more
+      "posthook_multi_runs_${index}" expression => "posthook_persistent_${index}.command_execution_${posthook_canon[${index}]}_failed",


I'd prefer a name like post_hook_rerun

peckpeck · 2017-07-17T18:45:34Z

techniques/fileDistribution/fileTemplate/1.0/fileTemplate.st



      #Case without execution

      "report_${index}" usebundle  => rudder_common_report("fileTemplate", "result_na", "${trackingkey[${index}]}", "Posthook", "${dst[${index}]}", "No post-modification needed to run"),
-      ifvarclass => "!file_modified_${index}.posthook_specified_${index}";
+      ifvarclass => "!file_modified_${index}.posthook_specified_${index}.!command_execution_${posthook_canon[${index}]}_reached";


I think there's a problem here, the posthook can be not reached if it has not yet been executed.

Sorry, I don't understand: if we erase this conditional state, persistent posthook in error will report as a N/A and not as a posthook execution.

A class can only be defined or not defined.
A class can be not defined, be be defined on next pass.
That's why using negation in class expressions is touchy.

But i see that this is enclosed in pass3, so there is no problem.

Fdall · 2017-07-19T10:49:15Z

Commit modified

Fdall · 2017-07-19T11:04:22Z

Commit modified

peckpeck · 2017-07-19T11:46:17Z

techniques/fileDistribution/fileTemplate/1.0/fileTemplate.st



      #Case without execution

      "report_${index}" usebundle  => rudder_common_report("fileTemplate", "result_na", "${trackingkey[${index}]}", "Posthook", "${dst[${index}]}", "No post-modification needed to run"),
-      ifvarclass => "!file_modified_${index}.posthook_specified_${index}";
+      ifvarclass => "!file_modified_${index}.posthook_specified_${index}.!command_execution_${posthook_canon[${index}]}_reached";


A class can only be defined or not defined.
A class can be not defined, be be defined on next pass.
That's why using negation in class expressions is touchy.

But i see that this is enclosed in pass3, so there is no problem.

peckpeck · 2017-07-19T11:46:32Z

techniques/fileDistribution/fileTemplate/1.0/fileTemplate.st

      "src_canon[${index}]" string => canonify("${relative_src}/${src[${index}]}");
      "dst_canon[${index}]" string => canonify("${dst[${index}]}");
      "temp_canon[${index}]" string => canonify("${temp}/${name[${index}]}");
      "temp_dir_canon" string => canonify("${temp}");
      "posthook_canon[${index}]" string => canonify("${posthook[${index}]}");

+      #Cancel persistent classes
+      pass3::


still there

peckpeck · 2017-07-19T11:49:31Z

...s/fileDistribution/fileTemplate/1.0/tests/test_error_fileTemplate/test_error_fileTemplate.cf

+    "other_files" slist => {"/tmp/aDirectory"};
+
+  methods:
+    "any" usebundle => command_execution("rm -f /tmp/${gen_files}");


there's a file_remove method for this

peckpeck · 2017-07-19T11:49:38Z

...s/fileDistribution/fileTemplate/1.0/tests/test_error_fileTemplate/test_error_fileTemplate.cf

+  methods:
+    "any" usebundle => command_execution("rm -f /tmp/${gen_files}");
+    "any" usebundle => command_execution("rm -f /var/rudder/templates/${src_files}");
+    "any" usebundle => command_execution("rm -Rf ${other_files}");


not yet for this

peckpeck · 2017-07-19T11:51:57Z

...fileDistribution/fileTemplate/1.0/tests/test_error_fileTemplate/test_error_fileTemplate.json

+                    {
+                      "var": {
+                        "name": "FILE_TEMPLATE_TEMPLATE",
+                        "value": "lmqoivuqiuvh"


use a value that is meaningful, like "not-a-file"

peckpeck · 2017-07-19T11:57:21Z

techniques/fileDistribution/fileTemplate/1.0/tests/test_fileTemplate/test_fileTemplate.cf

@@ -0,0 +1,3 @@
+#!/usr/local/bin/ncf -f 


If you have nothing to do, you should just have no init at all

Still to do

peckpeck · 2017-07-19T11:57:55Z

techniques/fileDistribution/fileTemplate/1.0/tests/test_fileTemplate/test_fileTemplate.json

+  "shortDescription": "",
+  "techniqueName": "fileTemplate",
+  "techniqueVersion": "1.0"
+}


peckpeck · 2017-07-19T12:01:57Z

techniques/fileDistribution/fileTemplate/1.0/tests/test_fileTemplate/test_fileTemplate.rb

+  its(:exit_status) { should eq 0 }
+end
+
+describe command("rm -f /var/rudder/tmp/templates/*") do


since the goal is just to clean up you can also remove all files in a single rm command

That way we can see easily which case failed in case of failure

This is just a cleanup code, not a test.
Moreover, the -f prevents rm from returning errors.

peckpeck · 2017-07-19T12:03:07Z

techniques/fileDistribution/fileTemplate/1.0/tests/test_posthook/test_posthook.json

+                    {
+                      "var": {
+                        "name": "FILE_TEMPLATE_TEMPLATE_POST_HOOK_COMMAND",
+                        "value": "/bin/ls /tmp/aDirectory"


Use a meaningful name

peckpeck · 2017-07-19T12:03:27Z

techniques/fileDistribution/fileTemplate/1.0/tests/test_posthook/test_posthook.rb

+  its(:stdout) { should match /^R: @@fileTemplate@@result_error@@.*?@@.*?@@.*?@@Posthook@@\/tmp\/persistentPosthooktest@@.*?The command .*?from postHook execution  could not be repaired$/m}
+end
+
+# Forced the execution of the posthook


Fdall · 2017-07-20T14:30:26Z

Commit modified

peckpeck · 2017-07-20T16:37:58Z

...s/fileDistribution/fileTemplate/1.0/tests/test_error_fileTemplate/test_error_fileTemplate.rb

+  it { should be_mode 750 }
+  it { should be_owned_by "root" }
+end
+


Still there

peckpeck · 2017-07-20T16:38:09Z

techniques/fileDistribution/fileTemplate/1.0/tests/test_fileTemplate/test_fileTemplate.cf

@@ -0,0 +1,3 @@
+#!/usr/local/bin/ncf -f 


peckpeck · 2017-07-20T16:39:14Z

techniques/fileDistribution/fileTemplate/1.0/tests/test_fileTemplate/test_fileTemplate.rb

+  its(:exit_status) { should eq 0 }
+end
+
+describe command("rm -f /var/rudder/tmp/templates/*") do


This is just a cleanup code, not a test.
Moreover, the -f prevents rm from returning errors.

peckpeck · 2017-07-20T16:40:00Z

techniques/fileDistribution/fileTemplate/1.0/tests/test_posthook/test_posthook.json

+                    {
+                      "var": {
+                        "name": "FILE_TEMPLATE_TEMPLATE_POST_HOOK_COMMAND",
+                        "value": "/bin/ls /tmp/aDirectory"


Fdall · 2017-07-21T16:46:08Z

Commit modified

Normation-Quality-Assistant · 2017-07-24T12:29:25Z

OK, merging this PR

peckpeck requested changes Jul 18, 2017

View reviewed changes

Fdall force-pushed the ust_11142/filetemplate_technique_posthook branch 2 times, most recently from 187efc5 to fba936d Compare July 19, 2017 11:04

peckpeck requested changes Jul 19, 2017

View reviewed changes

Fdall force-pushed the ust_11142/filetemplate_technique_posthook branch from fba936d to 967426c Compare July 20, 2017 14:30

peckpeck requested changes Jul 20, 2017

View reviewed changes

Fixes #11142: filetemplate technique posthook

b5c19be

Fdall force-pushed the ust_11142/filetemplate_technique_posthook branch from 967426c to b5c19be Compare July 21, 2017 16:46

peckpeck added the Ready for merge label Jul 24, 2017

Normation-Quality-Assistant merged commit b5c19be into Normation:branches/rudder/4.1 Jul 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #11142: filetemplate technique posthook #1177

Fixes #11142: filetemplate technique posthook #1177

Fdall commented Jul 17, 2017

peckpeck Jul 17, 2017

peckpeck Jul 17, 2017

peckpeck Jul 17, 2017

peckpeck Jul 19, 2017

peckpeck Jul 17, 2017

peckpeck Jul 17, 2017

peckpeck Jul 17, 2017

peckpeck Jul 17, 2017

Fdall Jul 19, 2017

peckpeck Jul 19, 2017

Fdall commented Jul 19, 2017

Fdall commented Jul 19, 2017

peckpeck Jul 19, 2017

peckpeck Jul 19, 2017

peckpeck Jul 19, 2017

peckpeck Jul 19, 2017

peckpeck Jul 19, 2017

peckpeck Jul 19, 2017

Fdall Jul 20, 2017

peckpeck Jul 20, 2017

peckpeck Jul 19, 2017

peckpeck Jul 19, 2017

Fdall Jul 20, 2017

peckpeck Jul 20, 2017

peckpeck Jul 19, 2017

peckpeck Jul 20, 2017

peckpeck Jul 19, 2017

Fdall commented Jul 20, 2017

peckpeck Jul 20, 2017

peckpeck Jul 20, 2017

peckpeck Jul 20, 2017

peckpeck Jul 20, 2017

Fdall commented Jul 21, 2017

Normation-Quality-Assistant commented Jul 24, 2017

Fixes #11142: filetemplate technique posthook #1177

Fixes #11142: filetemplate technique posthook #1177

Conversation

Fdall commented Jul 17, 2017

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

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

Fdall commented Jul 19, 2017

Fdall commented Jul 19, 2017

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

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

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

Fdall commented Jul 20, 2017

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

Fdall commented Jul 21, 2017

Normation-Quality-Assistant commented Jul 24, 2017