dird: fix RunScript parsing#358
Conversation
|
I cannot see why or where there is a bug introduced. The comment in the source that mentions the behaviour "Run on client by default" and the according line of code is present at least since the Bareos 12.4.1 release. This is what your PR changes. Since we handle long established default behaviour very seriously, I do not recommend to merge your PR. However, thanks for your contribution. What we should do next is:
|
|
Well, nevertheless it is a bug. I see, that you don't approve it, but why closing this PR already? |
54123a4 to
93f1a2b
Compare
|
As discussed: rebased on master, add systemtest. |
franku
left a comment
There was a problem hiding this comment.
The bug you discovered was caused by the former "tri-state" of the "target" value that previously could be nullptr, empty string or a string.
Nowadays the target value is not a char* (using POOLMEM*) but a std::string, thus the nullptr value is impossible. The problem appears because the empty string is actually a valid setting, whereas the std::string::empty() function is used to check if the string can be overwritten by a default value.
As far as I understood your solution sets the default value every configparser pass but allows the default string to be "overwritten" by a empty string.
IMHO the buggy behaviour won't affect users because almost most of them use the "run on client=yes" default setting. However, we should fix this.
I tried to run the python-bareos-test on my laptop but it fails. So we need to figure out why. I will come with a PN to your email and then we can discuss that. Therefore I chose "Request Changes".
This commit fixes a bug introduced during the recent months. Without this patch, all RunScripts are configured to "Runs On Client = Yes". Because of this, Admin and Console RunScripts did not work and are silently ignored. With this change, RunScripts can by set to run locally or on the Client (Runs On Client = Yes/No).
Add test class PythonBareosJsonRunScriptTest to check if RunScripts are executed in the correct context.
93f1a2b to
720b0a3
Compare
|
Implemented the requested changes and rebased on master. |
This commit fixes a bug introduced during the recent months.
Without this patch, all RunScripts are configured to "Runs On Client = Yes".
Because of this, Admin and Console RunScripts did not work and are silently ignored.
With this change, RunScripts can by set to run locally or on the Client (Runs On Client = Yes/No).
A systemtest for this is included in PR #314 .