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

Enhancement: Generate cookie cutter templates and apply linting on them #123

Merged
merged 6 commits into from
May 11, 2023

Conversation

wadhah101
Copy link
Contributor

@wadhah101 wadhah101 commented Apr 15, 2023

closes #122

@zhan9san zhan9san added the enhancement New feature or request label Apr 15, 2023
tools/generate-templates.sh Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
.ansible-lint Show resolved Hide resolved
tools/generate-templates.sh Outdated Show resolved Hide resolved
.ansible-lint Show resolved Hide resolved
requirements.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
.ansible-lint-ignore Show resolved Hide resolved
set -euo pipefail

rm -rf test/roles/* || true
mkdir -p test/roles
Copy link
Member

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 add test/roles in .gitignore?

Copy link
Contributor Author

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

Copy link
Member

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

What benefitsmktemp can bring?

IMHO, mktemp is better for unique file name. In this PR, we just want to create a directory and generate some files in them.

Copy link
Member

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.

Copy link
Contributor Author

@wadhah101 wadhah101 Apr 19, 2023

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 sadly

Copy link
Member

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 ?

Copy link
Contributor Author

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 file

Copy link
Member

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.

tox.ini Show resolved Hide resolved
@wadhah101
Copy link
Contributor Author

I will fix the remaining errors later and have a final commit for this PR @zhan9san

@zhan9san
Copy link
Member

@wadhah101

There are much many lint issues now. It's tedious to add them one by one in .ansible-lint-ignore.

We can add exclude_paths to exclude the whole folders.

In this PR, it is fine to enabl ansible-lint in CI only. Try to make the PR as smaller.

@apatard
Copy link
Member

apatard commented Apr 19, 2023

Can you please clean this PR a little bit ?
Current state doesn't make it easy to review (at least to me).

Moreover, I see changes which should probably need their own PR:

I'm going to add also some quick comments to some of the changes done. There will possibly more later, one this PR is clean.

.ansible-lint Show resolved Hide resolved
@wadhah101
Copy link
Contributor Author

@apatard I have cleaned up the code and only left changes relevant to making the ci functional.

Any syntax fixes for ansbile I will do in other PRs.

About VM size I plan to open another PR for it but no this one.

rm -rf test/roles/* || true
mkdir -p test/roles

while IFS='' read -r line; do DRIVER_NAMES+=("$line"); done < <(python "tools/toml_to_json.py" "pyproject.toml" | jq -rc '.project."entry-points"."molecule.driver" | keys[]')
Copy link
Member

Choose a reason for hiding this comment

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

why not making the python script outputting the list of drivers ?
This would avoid using jq and if we need later a way to get the list of drivers, the python script would be very handy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apatard Yeah at first i wanted to do it without a script with some shell command but couldn't find any reliable python-cli toml parser.
I have updated it to output the plugins, it makes more sense this way

@wadhah101 wadhah101 force-pushed the main branch 2 times, most recently from 87e2e40 to 8218e0b Compare May 9, 2023 18:57
@apatard
Copy link
Member

apatard commented May 10, 2023

I guess this PR is near to be ready to be merged. I hope that once merged, we'll manage to reduce the ignore list.

@apatard
Copy link
Member

apatard commented May 10, 2023

@zhan9san while we're waiting for github actions to be working again, do you have remaining comments ?

@zhan9san
Copy link
Member

LGTM.

Is Github Action down?

@wadhah101
Copy link
Contributor Author

Github actions seem to be back. Can any of the maintainers re run the checks ?

@apatard
Copy link
Member

apatard commented May 11, 2023

Things are not perfect but the CI is green, so I guess it's time to merge. The side-effects (if any) can be fixed later. It's better than not having -lint on the cookie cutter stuff. Thanks

@apatard apatard merged commit 7d8f040 into ansible-community:main May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ansible-lint for cookiecutter in CI jobs
3 participants