-
Notifications
You must be signed in to change notification settings - Fork 273
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
Fix linting issues #312
Fix linting issues #312
Conversation
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
@gardar If you could push a merge request with the first three commits (without "fix role name"), I'd merge that right away. Those three commits are not really isolated so they can be squashed together. |
ansible-lint refuses to run without fixing the role name first. |
I missed the draft status. Thx |
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
All issues fixed, are you sure about reverting the role name from |
It should be as it is in ansible galaxy right now. Since that one has all the history.
…________________________________
Van: gardar ***@***.***>
Verzonden: Tuesday, January 23, 2024 5:22:32 PM
Aan: riemers/ansible-gitlab-runner ***@***.***>
CC: Erik-jan Riemers ***@***.***>; Mention ***@***.***>
Onderwerp: Re: [riemers/ansible-gitlab-runner] Fix linting issues (PR #312)
All issues fixed, are you sure about reverting the role name<https://ansible.readthedocs.io/projects/lint/rules/role-name/> from gitlab_runner to gitlab-runner?
—
Reply to this email directly, view it on GitHub<#312 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAFGQOKGPJ3W4LMFPHUXGSLYP7PURAVCNFSM6AAAAABCGZ747WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBWGQZDIMBSHA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I see, the transition from hyphens to underscore in galaxy was some time ago and I think you can't make new roles with hyphens in the name. |
Don’t dare to touch it.. but I can ask if an alias is possible. But if it is that means people can steal other names too. So I doubt it. Going back and forth also doesn’t help ;)
…________________________________
Van: gardar ***@***.***>
Verzonden: Tuesday, January 23, 2024 5:40:27 PM
Aan: riemers/ansible-gitlab-runner ***@***.***>
CC: Erik-jan Riemers ***@***.***>; Mention ***@***.***>
Onderwerp: Re: [riemers/ansible-gitlab-runner] Fix linting issues (PR #312)
I see, the transition from hyphens to underscore in galaxy was some time ago and I think you can't make new roles with hyphens in the name.
Perhaps it's possible to get an alias in galaxy? So that both gitlab-runner and gitlab_runner names work?
—
Reply to this email directly, view it on GitHub<#312 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAFGQOMTGV2QPKO4GUPITTDYP7RXXAVCNFSM6AAAAABCGZ747WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBWGQ3TEMJYGA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Well since role names are now required to be with underscores and hyphens are only allowed for legacy purposes one would assume an alias would be standard practice. Let me know if you want to check with the galaxy team or I can change the metadata and add the role name linting rule to the ignore list. |
If it is possible to have both that would be fine too. I know a lot of people used the old role.. but it won’t be end of the world either.
…________________________________
Van: gardar ***@***.***>
Verzonden: Thursday, January 25, 2024 2:52:02 PM
Aan: riemers/ansible-gitlab-runner ***@***.***>
CC: Erik-jan Riemers ***@***.***>; Mention ***@***.***>
Onderwerp: Re: [riemers/ansible-gitlab-runner] Fix linting issues (PR #312)
Well since role names are now required to be with underscores and hyphens are only allowed for legacy purposes one would assume an alias would be standard practice.
I don't think anyone is able to steal your role name since it's prefixed with your namespace 😃
Let me know if you want to check with the galaxy team or I can change the metadata and add the role name linting rule to the ignore list.
—
Reply to this email directly, view it on GitHub<#312 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAFGQOL5RSO4SLIPNGNWDDLYQJPQFAVCNFSM6AAAAABCGZ747WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJQGI3DCMJZGA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Ok let me know if you get any response from the inquiry, otherwise I'll change the meta. |
In newer Ansible versions gitlab-runner throws errors, using _ in the role name is basically a requirement. Using the role_name value in meta helps address this without changing repository name. I believe Galaxy if I remember correctly now will reject it if there's a |
Maybe we can do both? I mean if it’s a requirement somewhere on the website and we should follow that pattern but then I loose the history / downloads from it too no?
…________________________________
Van: Eric Anderson ***@***.***>
Verzonden: Monday, February 12, 2024 7:56:17 PM
Aan: riemers/ansible-gitlab-runner ***@***.***>
CC: Erik-jan Riemers ***@***.***>; Mention ***@***.***>
Onderwerp: Re: [riemers/ansible-gitlab-runner] Fix linting issues (PR #312)
All issues fixed, are you sure about reverting the role name<https://ansible.readthedocs.io/projects/lint/rules/role-name/> from gitlab_runner to gitlab-runner?
In newer Ansible versions gitlab-runner throws errors, using _ in the role name is basically a requirement. Using the role_name value in meta helps address this without changing repository name.
—
Reply to this email directly, view it on GitHub<#312 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAFGQOPVKIITE37XCZFC7XLYTJQVDAVCNFSM6AAAAABCGZ747WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZZGM2DMNBZGI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I see two options, either to get the role renamed to gitlab_runner on galaxy, or if possible to add a alias on galaxy so that the role is available under both names. |
So what's the situation here, have you checked if the role can be renamed or if you can get a name alias? |
Been I’ll for some time and also busy as you are also ;) still have to do boring taxes and playing helpdesk for the family’s. I’ll need some time in the weekend and take some time for it.
…________________________________
Van: gardar ***@***.***>
Verzonden: Tuesday, February 27, 2024 5:02:40 PM
Aan: riemers/ansible-gitlab-runner ***@***.***>
CC: Erik-jan Riemers ***@***.***>; Mention ***@***.***>
Onderwerp: Re: [riemers/ansible-gitlab-runner] Fix linting issues (PR #312)
So what's the situation here, have you checked if the role can be renamed or if you can get a name alias?
—
Reply to this email directly, view it on GitHub<#312 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAFGQOJNKZRWCKEWG7VMTPLYVX7SBAVCNFSM6AAAAABCGZ747WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWHA4TKMRXGI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Although PRs are appreciated, if it sits for too long nothing happens. Can always update and do again :) This is just the automation talking |
Any news @riemers ? |
@gardar If you would split this PR to have only linting changes and not metadata changes like the role_name, I'll be happy to merge it right away. If you keep it as is, I don't want to touch it because we had troubles with Ansible-Galaxy before and this is too hot for me. |
Signed-off-by: gardar <[email protected]>
2f3b053
to
534541e
Compare
@@ -13,6 +13,7 @@ | |||
ansible.builtin.command: "{{ gitlab_runner_executable }} restart" | |||
listen: restart_gitlab_runner_macos | |||
become: "{{ gitlab_runner_system_mode }}" | |||
changed_when: true |
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 this necessary? Doesn't the command
module produces a "changed" anyway?
Could you please do a rebase onto master to get rid if the merge commit, because it seems when resolving the problems within the merge, some lines got lost (of a previous MR). To be on the safe side, please rebase then I'll review it again. |
f92d0b7
to
7f47d30
Compare
Signed-off-by: gardar <[email protected]>
And let me know when you are done, then I'll have another look at it |
@gardar when the conflicts are resolved and the branch is rebased, I'm happy to merge |
Thanks! Haven't had time to complete it yet, I'll give you a ping once it's ready. |
Although PRs are appreciated, if it sits for too long nothing happens. Can always update and do again :) This is just the automation talking |
Unstale |
Although PRs are appreciated, if it sits for too long nothing happens. Can always update and do again :) This is just the automation talking |
Unstale i suppose ;p |
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.
maybe we should close #342. the linting would already "fix" it. but not with optional dependencies.
See #344 |
fixes: #283