filebeat: fix pylint in test_registrar

[7AC]

No description provided.

[ruflin]

So far we are using autopep8 to do some "linting" in our python file. See https://github.com/elastic/beats/blob/master/Makefile#L64 for the command. In case we introduce pylint we should run it as part of the CI.

What is the difference between pylint and autopep8?

[7AC]

I haven't used autopep8 before but looks geared more towards style, pylint finds common coding mistakes much like a compiler would. This commit is incomplete I fixed 41 additional errors and 14 additional warnings, let me see what went wrong. Once I publish those the usefulness of pylint should be clearer. I'm all for adding it to CI but we need to fix the other files first :)

[ruflin]

I had a quick look at pylint and it seems both can check for pep8 but pylint can do some more stuff.

In the above change you added some flags on what should be ignore. I'm curious if we could have some global flags / config options on what should be ignored and what not to have a "definition" on what our current checks are and we don't have to add it to each file / method.

How much of the above changes were done automatically by pylint and how much had you to do manaully?

Few general notes:

• We try to be compatible with python 2.7 and 3.x if possible even though required version is 2.7
• In general the system test files have become messy over time as they have lots of repeated code inside. Probably lots of stuff could be simplified by introducing some shared methods. So if you touch lots of the code and see some patterns, feel free to simplify it
• In the past we discussed, if we should replace the system test code with golang. As the code base is large one sometimes more complex in Golang (my opinion) this will not happen very soon, but probably good to know.

[7AC]

All the changes were done manually, unlike autopep/goimports pylint doesn't auto-edit AFAIK. I could add a .pylintrc ignoring those warnings globally, but since I noticed only a few instances so far I disabled them locally so we can still fix those or at least not add new ones. You have a better understanding of what the overall python code looks like, let me know if you think we should just give up on some of them. The other (minor) reason I didn't add a .pylintrc is that until we enable it in CI the file will look useless.
Re Python 3.x do you see any change here that breaks compatibility?

[ruflin]

Autopep can automatically rewrite files to correct issue. See https://github.com/elastic/beats/blob/master/Makefile#L81 I assume pylint could do something similar?

I would suggest we add a .pylintrc file globally and ignore the warnings. But we should start fixing them and as soon as we have it fixed in all places, we should remove the warning. We have a similar approach to the Golang checks. There are a few things we already check in the CI like formatting, but we ignore missing comments. As soon as we have comments everywhere, we can add checks for it.

About python3: A few weeks ago I played around with some tools to figure out if there is a tool that can automatically check something is working in python 2.7 and python 3. But I wasn't successful so far. Figuring it out manually is quite hard. We have a habit of breaking python 3 from time to time ...

[7AC]

I figured if I test vs python3 CI will take care of python2.7 and we should be good right? Adding the .pylintrc sounds good I'll do it now.
About rewriting, pylint's warnings are generally about more convoluted cases so I doubt there's an option to rewrite automatically as it wouldn't be straightforward how to correct them.

[ruflin]

Not sure about the CI and python 2.7. CI does not execute all the scripts, for example the cherry pick one is not executed by CI.

[7AC]

I guess you're right, I forgot we don't run pylint in CI yet. I'll just test for both

[ruflin]

quite a file. Would it be possible to only have the variables inside which are not default? This makes it easier to spot where we don't follow the default behaviour.

[7AC]

I generated it with --generate-rcfile, one of the things I had to modify for instance was:

-method-rgx=[a-z_][a-z0-9_]{2,30}$
+method-rgx=[a-z_][a-z0-9_]{2,50}$

Would be hard to do that without a fully generated config, my guess is they generate it like that because there are many options like this. I agree it's not ideal though, let me try to simplify it a bit.

[ruflin]

LGTM