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

Temporary ignore ansible-lint var-naming[no-role-prefix] #276

Merged
merged 1 commit into from
May 24, 2023

Conversation

staticdev
Copy link
Collaborator

This error started with last update of ansible-lint. It triggers more than 80 errors. I am adding this ignore until we fix it.

@staticdev staticdev added the ci Continuous Integration label May 22, 2023
@staticdev staticdev requested review from aalaesar and wiktor2200 May 22, 2023 04:15
aalaesar
aalaesar previously approved these changes May 22, 2023
Copy link
Member

@aalaesar aalaesar left a comment

Choose a reason for hiding this comment

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

LGTM
but do we need to fix it ? 😄
I suppose It will come down to ansible galaxy linter that'll impact the collection stars level

@staticdev staticdev force-pushed the ci/temp-ignore-no-role-prefix branch 2 times, most recently from 1f7413e to f0108d4 Compare May 22, 2023 20:32
@staticdev
Copy link
Collaborator Author

LGTM but do we need to fix it ? smile I suppose It will come down to ansible galaxy linter that'll impact the collection stars level

I am pretty sure Galaxy does not update ansible-lint as fast as we do. But yes we should fix, I can do that soon. For now it will avoid annoying emails in our inboxes. Could you approve again? I did a failed test to fix.

@aalaesar
Copy link
Member

LGTM but do we need to fix it ? smile I suppose It will come down to ansible galaxy linter that'll impact the collection stars level

I am pretty sure Galaxy does not update ansible-lint as fast as we do. But yes we should fix, I can do that soon. For now it will avoid annoying emails in our inboxes. Could you approve again? I did a failed test to fix.

Actually, It reminds me of the issues I got with pre-commit hooks: #207 that got fixed with a simple update
some tests seems to be related to ansible lint version and maybe are disabled later.
IMHO, this lint rule seems pretty arbitrary.
I mean , enforcing naming convention of variable is pretty invasive and a very opiniatred rule (that some may want to apply)

I expect it to be disabled in a future version but for now we will deal with it with the lint ignore

@staticdev
Copy link
Collaborator Author

LGTM but do we need to fix it ? smile I suppose It will come down to ansible galaxy linter that'll impact the collection stars level

I am pretty sure Galaxy does not update ansible-lint as fast as we do. But yes we should fix, I can do that soon. For now it will avoid annoying emails in our inboxes. Could you approve again? I did a failed test to fix.

Actually, It reminds me of the issues I got with pre-commit hooks: #207 that got fixed with a simple update some tests seems to be related to ansible lint version and maybe are disabled later. IMHO, this lint rule seems pretty arbitrary. I mean , enforcing naming convention of variable is pretty invasive and a very opiniatred rule (that some may want to apply)

I expect it to be disabled in a future version but for now we will deal with it with the lint ignore

Hopefully yes or all roles using it will have to change all task names (bit big effort if you sum up for a not so big improvement).

@staticdev staticdev merged commit 646d746 into main May 24, 2023
@delete-merged-branch delete-merged-branch bot deleted the ci/temp-ignore-no-role-prefix branch May 24, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants