Reintroducing pylint

[agraul]

Modeled after the uyuni-rfc template, but not worthy of its own RFC. So let’s try a GH discussion.

Summary

pylint compliance will be a required check in Uyuni Pull Requests for Python files. Additonally, pylint will run on codestream branches on git push that touch Python files. Current failures are ignored, only new failures cause pylint to fail.

Motivation

pylint is able to enforce consistency for Python code and detect syntatic and semantic issues. We already have pylint jobs, but the way they are set up makes it very easy to miss them. Linting should be done as early as possible to enforce consistency, if possible before a change is merged.

pylint was not strictly enforced for our codebase and that shows, there are currently more than 2200 reported failures for backend/ that aren’t caused by import failures. Our repository layout adds another *~ 2400* import failures on top, but that is a problem for another day.

Detailed design

Fixing all failures reported by pylint is too much work, hence we start by enforcing pylint compliance for new or changed code only.

Ignoring current failures

We use pylint-ignore to maintain a list of known failures. The failures are saved in a machine- and human-readable Markdown file named pylint-ignore.md.

pylint-ignore.md is only updated manually, never automatically. For this reason, there is no make target that updates pylint-ignore.md. No new ignored pylint failures may be added to pylint-ignore.md, updates must only remove outdated ignored pylint failures.

Makefile.python defines the pylint target in two flavors. One that executes pylint-ignore inside a Docker container and one that is executed from inside such a container. This allows developers to run the same version of pylint-ignore in their development environment inside a container that is used in GitHub Actions.

pylint, without -ignore, can be executed manually inside the container using docker_shell target. This can be useful when someone wants to fix ignored pylint failures.

Iterative improvements

GitHub Actions run on pull_request and push events targeting codestream branches¹ that contain Python files. In either case pylint-ignore is executed on the changed files only. This ensures no time is wasted on unchanged files. https://github.com/Ana06/get-changed-files is a maintained GitHub Action that lists added / modified files and is used by the (new) Pylint workflow.

Contributors are encouraged to fix pylint failures that are ignored for each file touched in a Git commit, but not required to do so. Pull requests that only fix pylint failures require exhaustive test coverage, and are generally not accepted.

pylint-ignore compliance is mandatory for each Pull Request.

1: In Uyuni there is only one, master, but the policy can be used for SUSE Manager as well. In that case, the CI is not run by GitHub Actions but by an SUSE-internal Jenkins instance.

Drawbacks

• Making sure that code changes are in line with our style is work upfront. It pays later on, but initially it might annoy contributors
• We rely on a project that is maintained by one person. In my experience, that person is very responsive and already fixed two bugs I reported to him, but it’s a bus factor of 1.

Alternatives

No pylint

It’s what we have right now. Contributions are easy, but reading the code later on is hard. We also have a lot of unused code lying around and use bad practices. One consequence is that Python delevopers have a harder time grasping parts of our codebase than needed.

Strict pylint

No pylint-ignore, just pylint. This is a lot of work up front to get the check green, and we don’t have great test coverage to give use confidence to do large scale refactors. But without those refactors, the check will always be red and won’t provide any information.

Unsovled Questions

SUSE Manager CI

• Can Jenkins/Gitarro trigger on PRs/pushes only when specific files are changed?

Container needs recent Pylint

• Version on Leap 15.3 is too old for pylint-ignore
• pylint does not build in systemsmanagement:Uyuni:Utils/python-pylint

[agraul]

Please take a look and share your thoughts @meaksh @SchoolGuy @vzhestkov @juliogonzalez @jordimassaguerpla @mcalmer

[agraul]

Here is an initial pylint-ignore for backend/, without disabling import-error and no-name-in-module (happens a lot due to repository layout). It's too large for GitHub to render at this time, it must be viewed raw.

[agraul]

We will go forward with an implementation of the proposal as described plus ignoring the traditional stack for Uyuni. While this approach is not everyone's favourite, I think it is the best given the benefit it will bring without adding a huge overhead on the developers now. It is also not radically different from the other ideas brought up in this discussion, I hope everyone will be happy with it.

Thank you for the participation @SchoolGuy @vzhestkov @mcalmer @jordimassaguerpla @aaannz @sbluhm @meaksh !

[SchoolGuy]

I am fan of those tools, even if those changes break something. In the end I think that linters help enforce a common code flavor which is easily understandable for everyone then.

I would vote for the strict pylint and iterative approach. This means a lot of one time work but then we do two things with one tasks: tests and removing a lot of code smells. I did it with Cobbler and so far I feel that although some things break, the overall quality is improving.

The reason I would vote against the pylint-ignore approach is the fact that although it is a thin wrapper, it is an additional dependency. We already have a lot on our hands and with you guys helping out in Cobbler it will not get less.

[mcalmer]

pylint is known to produce different results just when changing the OS or python minor version.
This is one of the reasons why spacewalk gave up. Even a fixed pylint version did not fix this problem.
Has this changed?
If not we have only one chance to not back and force changing thing: containers with a specific OS, python and pylint.

[agraul]

That is interesting, I wasn't aware of that. I was already expecting to use a container, so this shouldn't be a problem. It makes sense to document "only use the container, preferably through the predefined make target".

Maybe it is better nowadays, but I don't know.

[agraul]

I would vote for the strict pylint and iterative approach. This means a lot of one time work but then we do two things with one tasks: tests and removing a lot of code smells. I did it with Cobbler and so far I feel that although some things break, the overall quality is improving.

Just to clarify the implications on this alternative:
pylint by itself can only ignore on a file and/or check level. We can disable unused-argument because we don't need to always use all, but then pylint won't tell us when we actually forget to use it. (A bit offtopic, but Google decided to solve this by using del arg_foo so it is explicitly "not used"). The same is true for a given file. pylint can either check a file or not, but it can't say "we know part of this huge file needs to be refactores, please only look at the three new methods I added".

As a consequence, we need to generate a list of disabled files to iteratively enable checking each file. This needs to be automated, else it would be easy to forget. This is something we need to come up ourselves I think. I didn't look for it specifically, but didn't run into such a thing when I was looking for something like pylint-ignore.

Then we have the implications when someone touches a file. The main difference is that one has to fix the whole file, e.g. backend/satellite_tools/reposync.py. Currently (ignoring import-error and no-name-in-module) there are 2 error, 30 warning, 20 refactor, and 24 convention type messages. You want to change one line? Cool, now fix those 76 messages.

I'm not saying that we shouldn't do it, but we should know that this is the (wanted) consequence.

As a last thought, we are accepting community pull requests, at least in theory. Our policy shouldn't make it too hard to accept contributions in my opinion.

[mcalmer]

We are about to deprecate the traditional stack. Does this investment make sense in this light?

I would vote for a list of files we ignore completly. Initial all files are part of this list.
Then we can start to fix the file and remove it from this list. With this we can decide at any point in time to stop fixing more linting errors.

Every new file would automatically be checked.

[agraul]

We are about to deprecate the traditional stack. Does this investment make sense in this light?

Good point, ignoring those completely sounds like a good idea.

Then we can start to fix the file and remove it from this list. With this we can decide at any point in time to stop fixing more linting errors.

I wonder if this is strict enough if we only use an "excluded_files_list" and pylint. I agree that there are files we can ignore (traditional stack, parts we never touch but are too afraid to delete), but what about files we want to fix? With pylint-ignore we would at least enforce new changes to files on that list to be pylint-compliant. I'm afraid no one wants to remove a given file from the list when they do bug fix, but that would keep the change from being linted.

The idea is similar to proposal with pylint-ignore.md I think. In both cases new files are checked completely and known failures are ignored. The difference is the level they are ignored at (whole file vs individual failures).

[mcalmer]

AFAIK there was some effort to create a global pylint job in our CI checking the whole git with 1 pylint call/file.
I think this is not what you have in mind. I am right?

[agraul]

Correct, the idea is to run pylint only on the subset of changed files. I think it makes sense to have a single pylintrc for the all of our Python code, but that can be done even if the pylint execution only targets a subset.

We could run pylint on everything, but that takes more time and the report should not change for untouched files anyway.

[jordimassaguerpla]

I would run pylint on uyuni as a github action but not in spacewalk.

First of all, because of what @mcalmer said, different versions of pylint will give you different results, which, a part from being a mess, adds no value on running a different stylecheck on spacewalk than on uyuni.

We could certainly run the exact same pylint if we build a containers that we reuse for testing in uyuni and spacewalk. However, again, what is the value of that? I mean, what is the chance that a backport from uyuni to spacewalk is done in a way that the style is wrong, and in that case, what is the problem with that? I understand we want to keep a sane codebase, for code quality (I am all in for this), with all the benefits this gives (maintenance, less probable to introduce bugs, easy to read by everyone, etc) ... for MASTER, given master is what future releases will be forked from. However, for a branch that has a an expiration date, which basically will get only backports .. I don't think is that relevant and worth the effort.

Thus, my opinion here is to implement a github action for uyuni.

[agraul]

You have a good point, I was mostly thinking about Uyuni and only mention SUMA to indicate we could do the same there. If I understood @juliogonzalez correctly, he wanted pylint to run everywhere and not only in Uyuni.

I mean, what is the chance that a backport from uyuni to spacewalk is done in a way that the style is wrong, and in that case, what is the problem with that?

I agree. We've also been "blind" in the last months (at least since I joined, I don't know about earlier than that), so introducing something for Uyuni is a net improvement -- we're not taking anything away from SUMA.

Thus, my opinion here is to implement a github action for uyuni.

If you want to check out my PoC that is based on the ideas behind this proposal, here it is: PR, Check

[meaksh]

I would vote for enabling this now only for Uyuni (since spacewalk branches has an expiration date, and different version might produce different results), and using pylint-ignore. This is already an improvement, since new code will be now passing pylint rules, it won't be hard for contributors, and won't take that much time from us. This would be also easy for developers since they can run the checks using the Makefile to run them inside a docker container.

At this moment, with our current resources I don't see we really want to go with strict linting or fixing everything at once. Of course, if we have time at some point, that would be cool. We could work on improving tests and fixing pylint from ignores files, but as a next step I would say.

Also investing lot of time/effors for fixing pylint issues from code that we're going to drop soon (tradictional stack), which is also poorly tested, is something I would try to prevent if possible.

[juliogonzalez]

I understood @juliogonzalez correctly, he wanted pylint to run everywhere and not only in Uyuni.

Keep in mind you are backporting stuff. Passing the linting at master doesn't seem passing the linting at any other branch. Specially with the backport is not just a cherrypick, but you have to merge conflicts, etc.

Still, I see @meaksh point, so it's up to you what to do.

[aaannz]

Personally I like the system salt has - pre-commit + black + pylint. It catches issues before being pushed to server and avoid rebases or additional commits just to fix linting or codestyle errors.

It needs some setup and getting familiar with it though.

[agraul]

I don't like pre-commit itself (rebases take too long, I only run it manually), but I like the combination of black, isort, pylint that Salt is using. I think that's a good combination use, but I'm not sure if we want how willing everyone is to introduce that all at the same time.

[sbluhm]

.