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

Remove lint command #3802

Merged
merged 1 commit into from Jan 5, 2023
Merged

Remove lint command #3802

merged 1 commit into from Jan 5, 2023

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Jan 5, 2023

No description provided.

@ssbarnea ssbarnea added enhancement deprecated marks deprecations labels Jan 5, 2023
@ssbarnea ssbarnea requested review from a team as code owners January 5, 2023 13:11
@ssbarnea ssbarnea requested review from greg-hellings, zhan9san, ganeshrn, trishnaguha and shatakshiiii and removed request for a team January 5, 2023 13:11
@github-actions github-actions bot added the bug label Jan 5, 2023
@zhan9san
Copy link
Contributor

zhan9san commented Jan 5, 2023

It's a good idea to slim the project.

It seems lint dependencies is missed.

https://github.com/ansible-community/molecule/blob/main/pyproject.toml#L68-L74

@ssbarnea
Copy link
Member Author

ssbarnea commented Jan 5, 2023

@zhan9san there are few missing bits as I just started but "lint" extra is not to be removed as its purpose is for internal use. What is funny is that even if we remove it, people will not observe because pip ignores any missing extras.

@gardar
Copy link
Contributor

gardar commented Jan 11, 2023

Wouldn't it be a good idea to put up a deprecation warning for a few versions before removing the option entirely?

@davedittrich
Copy link
Contributor

davedittrich commented Jan 11, 2023

Did this really need to be removed? Why can't a general lint phase remain, as it was, running a shell command? That command can be tox and any dependencies can be handled by the user. What is wrong with that? That has worked fine for years.

By removing the ability for molecule to perform linting each time tests are run, you require the user to add a Makefile to provide a dependent step (of linting) before continuing on to other steps.

@gardar
Copy link
Contributor

gardar commented Jan 11, 2023

Agreed!
Rather than removing it it would have been better if the lint step was moved into a playbook, so that it could be user defined like converge.yml , prepare.yml, cleanup.yml, etc.

I guess a possible workaround now, to be able to keep the linting within molecule and not relay on any external scripts, would be to add the linting into one of those playbooks.

@dnorthup-ums
Copy link

@gardar @davedittrich My general experience is that once Sorin has put something into place that's the end of it (even if most of us on the outside are surprised), so we'll just have to figure out how to live with this change that breaks >95% of CI / CD workflows in the real world. In the end, not a big deal...just an unexpected major annoyance.

@MarkusTeufelberger
Copy link
Contributor

Yeah, don't expect any communication or reasoning. A workaround might be to use the "shell" type dependency to run ansible-lint (and potentially other things) before the actual run.

hswong3i added a commit to alvistack/vagrant-debian that referenced this pull request Mar 29, 2023
hswong3i added a commit to alvistack/docker-php-fpm that referenced this pull request Mar 29, 2023
hswong3i added a commit to alvistack/docker-postgres that referenced this pull request Mar 29, 2023
hswong3i added a commit to alvistack/docker-ubuntu that referenced this pull request Mar 29, 2023
hswong3i added a commit to alvistack/vagrant-kubernetes that referenced this pull request Mar 29, 2023
hswong3i added a commit to alvistack/vagrant-centos that referenced this pull request Mar 29, 2023
hswong3i added a commit to alvistack/vagrant-fedora that referenced this pull request Mar 29, 2023
hswong3i added a commit to alvistack/vagrant-rhel that referenced this pull request Mar 29, 2023
hswong3i added a commit to alvistack/vagrant-opensuse that referenced this pull request Mar 29, 2023
hswong3i added a commit to alvistack/vagrant-ubuntu that referenced this pull request Mar 29, 2023
@retr0h
Copy link
Contributor

retr0h commented Apr 3, 2023

Molecule's workflow came about from legitimate "real-world" use-cases in Ansible role development. Built from the trenches, at a time RedHat was pushing ansible-test, disregarding it's history for "simplicity".

nre-ableton added a commit to nre-ableton/ansible-consul that referenced this pull request Apr 21, 2023
This is in preparation for Molecule 5.x, which removes the `lint`
command. For more information, see:

ansible/molecule#3802

Aside from that, this approach is also a bit nicer since linter errors
can be identified by their own GitHub checks run, which should be easier
for developers compared to the previous approach of having to hunt
through Molecule's error logs for linter errors. Also, it's a bit more
efficient since we only run the linters once per CI run instead of in
every Molecule scenario.
nre-ableton added a commit to nre-ableton/ansible-consul that referenced this pull request Apr 21, 2023
This is in preparation for Molecule 5.x, which removes the `lint`
command. For more information, see:

ansible/molecule#3802

Aside from that, this approach is also a bit nicer since linter errors
can be identified by their own GitHub checks run, which should be easier
for developers compared to the previous approach of having to hunt
through Molecule's error logs for linter errors. Also, it's a bit more
efficient since we only run the linters once per CI run instead of in
every Molecule scenario.
@madrover
Copy link
Contributor

So, what's the alternative now?
Linting as a part of the role development process is a very handy feature that helps to improve the code quality.
@ssbarnea , it would be great to get some insight about the rationale of this breaking change and what alternative has been devised by the development team...
Thanks!

@dnorthup-ums
Copy link

@madrover At $dayjob we'll likely be implementing an additional linting stage to the CI/CD flow that gets run on every push / merge request / and merge operation on our ansible repositories. Many places use git-hooks, both pre- and post-commit to enforce linting (but please read the manual before doing that, for the sake of your sanity). Additionally, many editors these days support on-the-fly linting via plugins (EG: VSCode and the dearly departed Atom). There are other viable options, this change was just rather unexpected outside of the core maintainer group.

@ziegenberg
Copy link
Contributor

For a short explanation of why lint was removed see here: #3825 (reply in thread)

@madrover
Copy link
Contributor

Thanks for the insights. I understand that this can be easily overcome on a CI/CD workflow and/or using other techniques, although it's annoying to have to tweak all the existing pipelines, but we also use Molecule intensively as a local development tool and having the linting capability built in was very nice.

I can understand the rationale described at #3825 (reply in thread) but removing all the functionality altogether is probably not the most thoughtful approach for the users. IMHO, this functionality could have been disabled by default, thus solving the described issues, but left there for users that rely on it.

In a nutshell, while I don't disagree with the rationale of not running linting, I think that making it toggleable and disabled by default would have achieved the same result and would have been less onerous for users.

@davedittrich
Copy link
Contributor

@gardar @davedittrich My general experience is that once Sorin has put something into place that's the end of it (even if most of us on the outside are surprised), so we'll just have to figure out how to live with this change that breaks >95% of CI / CD workflows in the real world. In the end, not a big deal...just an unexpected major annoyance.

From what I have seen in digging through code, I believe the linter worked perfectly fine until an attempt was made to produce fancy colored output, which didn't do encoding properly (hence the "c h a r a c t e r s i n t h e o u t p u t" looked funny). Multiple attempts to fix this, or detect stdin being a tty or not, did not work because they weren't fully thought through, and attempts by outsiders to issue PRs that made the bugs clear were rejected. So instead of backing out all the buggy changes, the "solution" seems to be to remove the functionality entirely. That makes a developers life great, but results in a terrible user experience. Sure I can add a Makefile to perform the linting apart from molecule (which is designed to make such checks easy, ironically) and it will work, but not something everyone using the system will want to learn.

I also noticed that this discussion involved pip dependencies that were/were not present in various use cases. Switching to poetry would improve the situation, or using additional requirements files instead of just one.

In summary, the pace of changes in this (and related) projects seems to be too high, since a non-trivial number of those changes produce regression problems, give the user no time to deal with deprecations, or introduce latent bugs that add to maintenance costs later on. I'd prefer fewer changes, more carefully designed and following a test-driven development methodology that exposes regression problems earlier, and working more closely with people contributing PRs or capable of multi-repo debugging.

robin-checkmk added a commit to Checkmk/ansible-collection-checkmk.general that referenced this pull request May 2, 2023
svor added a commit to devspaces-samples/ansible-devspaces-demo that referenced this pull request May 18, 2023
Signed-off-by: Valerii Svydenko <vsvydenk@redhat.com>
robin-checkmk added a commit to Checkmk/ansible-collection-checkmk.general that referenced this pull request May 22, 2023
@fkuep
Copy link

fkuep commented Jul 13, 2023

@ssbarnea

_ No description provided. _

We are asking frequently.
At least put it in the faq https://ansible.readthedocs.io/projects/molecule/faq/ .

@jimboolio
Copy link

jimboolio commented Sep 19, 2023

I spent hours trying to debug my molecule installation before I landed here. There are still plenty of resources which hint that such feature exists and when searching for the official docs, it returns some results, from the count of which you may guess linting is not there anymore, but I feel like it would be very good to have something - anything confirming that this doesn't exist. This issue is already quite buried in github so I think finding this information out is difficult.

Just to provide context, I was quickly able to find some examples of configuring molecule linting from web search so it is totally reasonable someone else has experienced this frustration:
https://opensource.com/article/18/12/testing-ansible-roles-molecule
https://www.jeffgeerling.com/blog/2018/testing-your-ansible-roles-molecule

Also, on molecule version 6, if one goes and adds the lint step to their test sequence, molecule does not in any form indicate that this feature is removed. It crashes in such situations. Also, when running molecule lint it doesn't indicate that the command is deprecated, it just says that it doesn't exist.

I'm just getting started with molecule, while I do not like linting not being there anymore, I do understand the reasoning here. Additionally I feel like this thread was at least somewhat informational resource for finding a workaround, so it would provide value as linked to the docs if some mention is at some point added. Otherwise we have to pray for favorable SEO.

@serpro69
Copy link

Molecule docs could really be improved to indicate that this has been deprecated. Searched the web for an hour or so before I landed here...
I guess one of the workarounds at the moment is to include linting via dependency with shell type. Ugly, but works (kind of). If only there was a way to configure retries... 🤦

serpro69 added a commit to serpro69/ansible-collection-devxp that referenced this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated marks deprecations major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet