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

Create template_instead_of_copy.py #2346

Merged
merged 6 commits into from Sep 16, 2022

Conversation

GhostLyrics
Copy link
Contributor

I'm sending a PR in response to a request to upstream a rule that complains about templating inside of ansible.builtin.copy. I'm sorry, I did not know how to decide on severity nor did I know how to find out in which version of Ansible that warning was added to the documentation.

Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

First, thanks for contributing this new rule. Here are few things missing:

  • no tests included
  • no markdown documentation included, create a template-instead-of-copy.md file in the same directory as the rule and try to match the same format used from the other rules, with before/after examples.
  • rules should trigger only if user tries to use jinja inside content, not on just the presence of content key.

@GhostLyrics
Copy link
Contributor Author

GhostLyrics commented Aug 26, 2022

Here are few things missing:

  • no tests included

Will check.

  • no markdown documentation included, create a template-instead-of-copy.md file in the same directory as the rule and try to match the same format used from the other rules, with before/after examples.

Will check.

  • rules should trigger only if user tries to use jinja inside content, not on just the presence of content key.

IMHO, this is only part of the solution. Thinking about the comment and the linked threads, I probably want to trigger if at least one of the following is true:

  • jinja context
  • new lines via escape (\n, etc).

What do you think?

@cognifloyd
Copy link
Member

Please add tests. That will make your target use cases easier to understand.

Please make sure you have a test that shows content: with jinja that skips this rule using a comment. I have some self contained playbooks that must not use any other files based on how I distribute/run them (in some kubernetes pods managed by a larger workflow engine). So, in general, this rule sounds good, but I need to make sure I can ignore it in those cases.

@ssbarnea ssbarnea added the incomplete Additional work or information is required label Aug 30, 2022
ssbarnea and others added 3 commits September 16, 2022 19:38
- add tests
- fix incorrect trigger for content without variables
- add documentation
@ssbarnea ssbarnea removed the incomplete Additional work or information is required label Sep 16, 2022
@ssbarnea ssbarnea merged commit fdc216d into ansible:main Sep 16, 2022
@bendem
Copy link
Contributor

bendem commented Oct 5, 2022

Items which are templated should use template instead of copy with content to ensure correctness.

I'm kind of confused about this comment. What could be "incorrect" from using copy[content] instead of template?
I can understand it's better taste (though that's an opinion), but I don't understand how it could be incorrect. Am I missing something?

EDIT: Nvm, I missed the rewrite because it hasn't been released yet.

@ssbarnea
Copy link
Member

ssbarnea commented Oct 5, 2022

@bendem Ping me back if the current version is still unclear, I can ask someone else to produce even more detailed examples and/or explanation. From what I understood, the difference is subtle, but it is. That rule is not about "style" preference, is about the risk of getting an unexpected outcome.

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

Successfully merging this pull request may close these issues.

None yet

4 participants