Skip to content
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 #18009: File in INI section method does not work as intended #1233

Open
wants to merge 3 commits into
base: branches/rudder/5.0
Choose a base branch
from

Conversation

Fdall
Copy link
Contributor

@Fdall Fdall commented Aug 5, 2020

@Fdall Fdall requested a review from amousset August 5, 2020 16:05
"sanitize" usebundle => _classes_sanitize("${class_prefix}");
"sanitize" usebundle => _classes_sanitize("${old_class_prefix}");
"report" usebundle => _log_v3("Insert line(s) into ${file}", "${file}", "${old_class_prefix}", "${class_prefix}", @{args});
}

bundle edit_line append_block(block) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these bundle should go in the ncf lib

"section_and_blank_line" string => "[${section}]
";
"hash" string => hash("${report_param}", "md5");
"last_line" string => execresult("/usr/bin/tail -n 1 ${file} #${hash}", "useshell");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure this path is the same every platform. Is it ?

…nded

Fixes #18009: File in INI section method does not work as intended
@Fdall
Copy link
Contributor Author

Fdall commented Aug 10, 2020

PR updated with a new commit

@@ -34,32 +34,60 @@ bundle agent file_line_present_in_ini_section(file, section, line)
"report_param" string => join("_", args);
"full_class_prefix" string => canonify("file_line_present_in_ini_section_${report_param}");
"class_prefix" string => string_head("${full_class_prefix}", "1000");
"section_and_blank_line" string => "[${section}]
";
"hash" string => hash("${report_param}", "md5");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems overkill. file and section should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can have 2 edit in the same sections and file in 2 directives, this is fine


pass2::
"line_list" slist => splitstring("${line}", "${const.n}", "999999");
"unique_lines" slist => unique("line_list");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't understand why this is deduplicated. nowhere in the method it is stated that it should behave like that, and this is really surprising for user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the current behaviour, it currently only uses "insert_lines" promises so you can't have 2 times the same line in your section. And here it done manually since the section delimiters are limited in cfengine and do not work if the section is the last one in a file and is empty, the section automatically becomes the whole file.... so we need to create a block of text, clean its duplicated lines and insert it at the end without any duplication check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was ncf_insert_block, which is insert_type => "preserve_block", so it doesn't deduplicate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an issue: at section creation it used ncf_insert_block, with preserver_block (which in fact still do a dedupolicate but with the whole block and not line per line), but afterward, when the section exist, it uses "ensure_line_in_ini_section" https://github.com/Normation/ncf/blob/fc517e756083b0d05f44001d17f01e785b3a2f27/tree/30_generic_methods/file_line_present_in_ini_section.cf#L57

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@Fdall Fdall Aug 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And does apply a "deduplicate" which makes it inconsistent

ifvarclass => "section_absent",
comment => "Add section to file with a blank line";
comment => "Add lines at the end of the file since the target section exists, is empty and is the last one in the file",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment should state "since target seection doesn't exists"

…as intended

Fixes #18009: File in INI section method does not work as intended
@Fdall
Copy link
Contributor Author

Fdall commented Aug 24, 2020

PR updated with a new commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants