Bug #2819
closed
checkGenericFileContent/3.0 technique requires content to add
Description
The checkGenericFileContent/3.0 technique (Enforce a file content) describes "Content of the file" as optional, but if not specified, it inserts an unexpanded '$(generic_file_content_payload1)' variable at the end of the file.
This is in Rudder 2.4 beta2.
Files
Updated by Matthieu CERDA over 12 years ago
- Status changed from New to In progress
- Assignee changed from Jonathan CLARKE to Matthieu CERDA
- Priority changed from N/A to 1 (highest)
- Target version set to 2.4.0~beta4
Aaaaah... checkGenericFileContent our most complete yet troublesome Technique :) I'll take a look at that, thanks for the report !
Updated by Michael Gliwinski over 12 years ago
Sorry, the variable got mangled by redmine, it inserts: $(generic_file_content_payload[1])
Updated by Matthieu CERDA over 12 years ago
- Status changed from In progress to Discussion
Michael, do you happen to have a "Enforce the content of the file" tick box at the top of your Directive by any chance ?
Updated by Michael Gliwinski over 12 years ago
Matthieu CERDA wrote:
Michael, do you happen to have a "Enforce the content of the file" tick box at the top of your Directive by any chance ?
No, it's unticked. Only "Enable the replacement of lines using a regexp" is ticked.
Updated by Matthieu CERDA over 12 years ago
Oh well then. It's a bug. Let's see this ...
Updated by Matthieu CERDA over 12 years ago
- Status changed from Discussion to In progress
OK, bug confirmed, the "Enable the replacement of lines using a regexp" tick box seems to fail. I'm trying to fix this.
Updated by Michael Gliwinski over 12 years ago
Matthieu CERDA wrote:
OK, bug confirmed, the "Enable the replacement of lines using a regexp" tick box seems to fail. I'm trying to fix this.
Just to clarify, the replacement using regex does actually work for me when ticked, it's just it's still trying to insert a line even if content is not specified. Is that the same thing you were seeing?
Updated by Matthieu CERDA over 12 years ago
- % Done changed from 0 to 40
Yes. It is because we never tested this Technique leaving the file content text area empty. If empty, the variable is not expanded (because considered non-existent) and is inserted nevertheless, thus leading to this unwanted behavior.
During the time for the fix, I have a quick'n'dirty workaround: Add a simple comment with this, a simple "# This is a comment" would be enough to make things straight.
I'm modifying the Technique to detect an empty field and ignore it.
Updated by Michael Gliwinski over 12 years ago
Matthieu CERDA wrote:
During the time for the fix, I have a quick'n'dirty workaround: Add a simple comment with this, a simple "# This is a comment" would be enough to make things straight.
That's exactly what I've done :)
Thanks!
Updated by Matthieu CERDA over 12 years ago
- File checkGenericFileContent.cf checkGenericFileContent.cf added
- Assignee changed from Matthieu CERDA to Nicolas PERRON
- % Done changed from 40 to 60
Handing that to NPE, since I'm not currently "on duty".
Here is attached an example of a file that resolves the problem, and I will explain here what I did:
- Added a class expression that detects if "generic_file_content_payload[index]" is a variable, to see if something has been input as a payload
- Added an "ifvarclass => "edit_content_$(index)"," on the first file edition statement
- Added a special report in case the file edition is not needed:
"@@checkGenericFileContent@@result_success@@$(generic_file_content_uuid[$(index)])@@File@@$(generic_file_content_path[$(index)])@@$(g.execRun)##$(g.uuid)@#The file $(generic_file_content_path[$(index)]) has no defined payload. Skipping line insertion..."
ifvarclass => "!modify_lines_$(index)";
Updated by Matthieu CERDA over 12 years ago
To put things simple:
- Detect if the payload is defined (if something has been entered in the file edition textarea)
- If yes, add the content to the file, if not skip the addition.
- If not, report a success saying that the file edition line addition is not needed. (no payload)
Updated by Jonathan CLARKE about 12 years ago
- Target version changed from 2.4.0~beta4 to 2.4.0~rc1
Updated by Nicolas PERRON about 12 years ago
This bug is not forgotten.
As Matthieu have found a easy workaround we have postponed it from 2.4.0~beta4 to 2.4.0~beta6 to focus on other tasks.
I will work on it as soon as possible !
Updated by Nicolas PERRON about 12 years ago
- File 0001-Fixes-2819-Add-classes-to-prevent-replacement-of-lin.patch 0001-Fixes-2819-Add-classes-to-prevent-replacement-of-lin.patch added
- Status changed from In progress to Discussion
- Assignee changed from Nicolas PERRON to Jonathan CLARKE
Here is a patch.
This is a fix for this bug on branch 2.4 but I'm not sure about the branch. Jon, can you confirm that this bug should be fixed on branch 2.4 ? Shouldn't be fixed on branch 2.3 ?
Updated by Jonathan CLARKE about 12 years ago
- Assignee changed from Jonathan CLARKE to Nicolas PERRON
Nicolas PERRON wrote:
Here is a patch.
This is a fix for this bug on branch 2.4 but I'm not sure about the branch. Jon, can you confirm that this bug should be fixed on branch 2.4 ? Shouldn't be fixed on branch 2.3 ?
2.3 branch is in maintenance for 6 months from when 2.4 is released, so any bugs should be fixed in 2.3 if they apply there. So it's simple: if this bug is present in 2.3, it should certainly be fixed there!
Updated by Nicolas PERRON about 12 years ago
First of all, this bug is reproducible on branch 2.3, so this is not only linked to 2.4.0~beta6.
Secondly, the patch i've added to this ticket fix the bug in the way that "No value in 'String used as a replacement (optional)' amounts to not change the line in the file" but this is not the initial behaviour of the Technique.
Finally, the patch is completely useless as soon as we have two Directives on the same Rule because it seems that this bug is mostly related to #3014
Updated by Nicolas PERRON about 12 years ago
- Status changed from Discussion to Pending technical review
- % Done changed from 90 to 100
Applied in changeset commit:6113a5851d1e1e581f772417b96ca13303883ddd.
Updated by Jonathan CLARKE about 12 years ago
- Status changed from Pending technical review to Released
Looks good to me, thanks Nicolas and Nicolas!