From 6f96ac271b3bb386323cbb5b5a12c3810ad13e5c Mon Sep 17 00:00:00 2001
From: Sorin Sbarnea <sorin.sbarnea@gmail.com>
Date: Wed, 28 Sep 2022 22:23:42 +0100
Subject: [PATCH] template-instead-of-copy: reduce number of false positives

- adds extra conditions for triggering the rule, basically eliminating
  the most simple strings from triggering it
- update documentation

Fixes: #2501
---
 .../rules/template-instead-of-copy.md         | 34 ----------
 .../rules/template_instead_of_copy.md         | 67 +++++++++++++++++++
 .../rules/template_instead_of_copy.py         | 11 ++-
 3 files changed, 77 insertions(+), 35 deletions(-)
 delete mode 100644 src/ansiblelint/rules/template-instead-of-copy.md
 create mode 100644 src/ansiblelint/rules/template_instead_of_copy.md

diff --git a/src/ansiblelint/rules/template-instead-of-copy.md b/src/ansiblelint/rules/template-instead-of-copy.md
deleted file mode 100644
index 2d4631a8d1b..00000000000
--- a/src/ansiblelint/rules/template-instead-of-copy.md
+++ /dev/null
@@ -1,34 +0,0 @@
-# template-instead-of-copy
-
-This rule identifies the presence of variable interpolation inside `copy`
-module and recommends instead the use of the `template` module. This is based on
-the official [recommendation](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/copy_module.html#synopsis)
-from Ansible documentation.
-
-## Problematic Code
-
-```yaml
----
-- name: Example playbook
-  hosts: localhost
-  tasks:
-    - name: Write file content
-      ansible.builtin.copy:
-        content: "Some {{ foo }}" # <-- should use template instead
-        dest: /tmp/foo.txt
-```
-
-## Correct Code
-
-```yaml
----
-- name: Example playbook
-  hosts: localhost
-  tasks:
-    - name: Write file content
-      ansible.builtin.template:
-        src: foo.txt.j2
-        dest: /tmp/foo.txt
-```
-
-Also, create a file `templates/foo.txt.j2` with `Some {{ foo }}` as its content.
diff --git a/src/ansiblelint/rules/template_instead_of_copy.md b/src/ansiblelint/rules/template_instead_of_copy.md
new file mode 100644
index 00000000000..e2805929d69
--- /dev/null
+++ b/src/ansiblelint/rules/template_instead_of_copy.md
@@ -0,0 +1,67 @@
+# template-instead-of-copy
+
+This rule identifies the presence of variable interpolation inside `copy`
+module and recommends instead the use of the `template` module. This is based on
+the official [recommendation](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/copy_module.html#synopsis)
+from Ansible documentation.
+
+This rule will be triggered if any of the following conditions are met:
+
+- `content` is not a string
+- `content` contains any Unicode character (not being pure ASCII)
+- `content` is it contains newline chars in any place other than the end of file
+- `content` contains at least one of `{{`, `{%`, `{#` substrings
+
+```{warning}
+Unless you explicitly include a `\n` at the end of your string, the final
+file will not have a trailing newline. This is a very common error, as many
+tools fail to work properly if the last line is not terminated by a newline.
+```
+
+## Problematic Code
+
+```yaml
+---
+- name: Example playbook
+  hosts: localhost
+  tasks:
+    - name: Write file content
+      ansible.builtin.copy:
+        content: { "foo": "bar" } # <-- should use template instead
+        dest: /tmp/foo.txt
+```
+
+Please note that in this case, the content of the file will be
+`{ "foo", "bar" }`, which is different from the original content (see that
+the single quotation marks were modified by Jinja into double ones). The file
+will also not have an ending newline, something that is usually expected by
+people knowing Python language.
+
+Several other conversions can happen while using `copy`, including up to 4
+levels of templating. On the other hand, the `template` module will only
+perform 1 level of templating.
+
+## Correct Code
+
+```yaml
+---
+- name: Example playbook (1st solution, using `template` module)
+  hosts: localhost
+  tasks:
+    - name: Write file content
+      ansible.builtin.template:
+        # Add `templates/foo.txt.j2` with what you need inside
+        src: foo.txt.j2
+        dest: /tmp/foo.txt
+```
+
+```yaml
+---
+- name: Example playbook (2nd solution, use only a very simple string)
+  hosts: localhost
+  tasks:
+    - name: Write file content
+      ansible.builtin.copy:
+        content: "{ 'foo': 'bar' }"
+        dest: /tmp/foo.txt
+```
diff --git a/src/ansiblelint/rules/template_instead_of_copy.py b/src/ansiblelint/rules/template_instead_of_copy.py
index 97a359957b8..83c74f352d7 100644
--- a/src/ansiblelint/rules/template_instead_of_copy.py
+++ b/src/ansiblelint/rules/template_instead_of_copy.py
@@ -34,7 +34,16 @@ def matchtask(
         """Confirm if current rule is matching a specific task."""
         if task["action"]["__ansible_module__"] == "copy":
             content = task["action"].get("content", "")
-            if "{{" in content:
+            if not isinstance(content, str):
+                return True
+            for seq in ("{{", "{#", "{%"):
+                if seq in content:
+                    return True
+            try:
+                content.encode("ascii")
+            except UnicodeEncodeError:
+                return True
+            if "\n" in content.rstrip("\n"):
                 return True
         return False