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

Apply var-spacing tests to vars files #2063

Merged
merged 3 commits into from Apr 29, 2022

Conversation

notok
Copy link
Contributor

@notok notok commented Apr 2, 2022

Currently, var-spacing rules are applied only to variables on tasks.
I think the rule should also be applied to vars files (like files in role/defaults/).

I wrote the code referring var-naming rule code.
Line numbers could not be obtained when using matchyaml, so added jq-style path to the variable.
But the code may be a bit tricky. Any suggestions are welcome.

Fixed code to get line number from AnsibleUnicode.

@notok notok force-pushed the var_spacing_for_vars_file branch 2 times, most recently from 8a50c18 to 9f72276 Compare April 2, 2022 10:41
@notok
Copy link
Contributor Author

notok commented Apr 2, 2022

eco test is failing, but this is correctly detected lint error.

I sent PR to fix this to debops repo.

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.

This looks ok but it does not include a positive and negative test, something essential for adding it. Can you please add one?

@ssbarnea ssbarnea added the bug label Apr 6, 2022
@notok notok marked this pull request as draft April 7, 2022 23:31
@notok
Copy link
Contributor Author

notok commented Apr 7, 2022

@ssbarnea Thank you for your comment.

I'm adding tests, but I found a problem that noqa does not work for vars files. (This is the same for var-naming rule..)
I'll re-check the code.

@notok notok force-pushed the var_spacing_for_vars_file branch 2 times, most recently from d5651fa to 2fe69d4 Compare April 7, 2022 23:52
@notok notok force-pushed the var_spacing_for_vars_file branch from 2fe69d4 to 46d5494 Compare April 10, 2022 11:28
@notok notok marked this pull request as ready for review April 10, 2022 12:19
@notok
Copy link
Contributor Author

notok commented Apr 10, 2022

I added test for vars file, and fixed the code to make noqa working.

@ssbarnea Could you please review again?

@cognifloyd cognifloyd enabled auto-merge (squash) April 28, 2022 14:41
@ssbarnea ssbarnea disabled auto-merge April 29, 2022 06:29
@ssbarnea
Copy link
Member

ssbarnea commented Apr 29, 2022

@notok @cognifloyd When making behavior change it is likely for the eco pipeline to fail as the output of linting on those repos will be different. What you need to do is to run locally tox -e eco and to include the modified files with your change.

I did it for you but I wanted to mention it for future changes. This gives us the opportunity to spot if the changed behavior is buggy or not.

@ssbarnea ssbarnea merged commit 1717bcd into ansible:main Apr 29, 2022
@notok
Copy link
Contributor Author

notok commented Apr 30, 2022

@ssbarnea Thanks for your help. I now understood how to. I will do it next time.

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

Successfully merging this pull request may close these issues.

None yet

3 participants