Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Enhancement: Generate cookie cutter templates and apply linting on them #123
Enhancement: Generate cookie cutter templates and apply linting on them #123
Changes from 5 commits
956fae8
e751862
fcafedf
d3c16c4
39539e8
270914c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every time we run
tox -e lint
, it will generate some temporary files locally, can we addtest/roles
in.gitignore
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre-commit will ignore stuff inside .gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be missing something but why not creating a temporary directory with mktemp ? issue with non linux os like macosx ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What benefits
mktemp
can bring?IMHO,
mktemp
is better forunique
file name. In this PR, we just want to create a directory and generate some files in them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it cleaner. It won't do any change in the working directory and it should avoid having to deal with configurations files of tools like git or pre-commit to ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some errors in the generated molecule template that I have no way around. which requires me to add the files to
.ansible-lint-ignore
.I prefer to use
mktemp
but.ansible-lint-ignore
doesn't take wildcard paths sadlyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm. maybe it would be easier to fix all errors before merging this PR, maybe this would make the .ansible-lint-ignore file useless ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some errors are unfixable. And the fixable ones are too much to be included in this PR. there's no going around
.ansible-lint-ignore
fileThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fun to fix the real lint issue in another issue.
Ignoring them to make the PR smaller is acceptable.