Consolidate how we run linters

[ssbarnea]

Ensure we use pre-commit as linter orchestrator as this ensures that
we always use the same linter version.

[Shrews]

What is this "consolidating" with? I'm not sure I see the improvement here. If the only reason is to guarantee we always use the same version (and I'm not convinced we should), we can just set the version in test/requirements.txt.

[ssbarnea]

First, requirements.txt is for runtime requirements, not test ones. pre-commit takes care of version locking itself and can automatically make pull requests to update the linters, see how it works on devtools project, example ansible/ansible-lint#2229

Also you do not want to get different results when you run 'pre-commit' or 'tox -e lint'. At this moment that is what it happens.

You can ask @cidrblock or we can even setup a meeting, I would love to explain why we ended up with that approach.

In regards to pinning test dependencies we also went for a very interesting approach, using contraint files and having dependabot and/or pip-compile update them and create pull-requests for them. The main advantage of this is that the release of breaking dependency will not break your CI/CD pipelines, it will only produce a pull-request to update contraints that fails, but other pending changes will not be affected by it.

[Shrews]

First, requirements.txt is for runtime requirements, not test ones. pre-commit takes care of version locking itself and can automatically make pull requests to update the linters, see how it works on devtools project, example ansible/ansible-lint#2229

That's not accurate. Our tox.ini installs the requirements from test/requirements.txt into the test tox environment before the tests run. The top-level requirements.txt file (also installed to each tox test environment) is for the runtime requirements.

Also you do not want to get different results when you run 'pre-commit' or 'tox -e lint'. At this moment that is what it happens.

I'm not sure why you see that. We don't tend to use any pre-commits in our workflow, so none of the Executor team has experienced that. If it's because of the existing .pre-commit-config.yaml file in our repo, I'd be more in favor of just removing that file. Not even sure why that exists, tbh.

You can ask @cidrblock or we can even setup a meeting, I would love to explain why we ended up with that approach.

In regards to pinning test dependencies we also went for a very interesting approach, using contraint files and having dependabot and/or pip-compile update them and create pull-requests for them. The main advantage of this is that the release of breaking dependency will not break your CI/CD pipelines, it will only produce a pull-request to update contraints that fails, but other pending changes will not be affected by it.

Appreciate the info on the devtools workflow. Maybe a meeting between the Executor and Devtools teams should be done first if you want to explain your workflow and encourage Executor to adopt it. I'm not sure that just submitting PRs to change the current Executor workflow is the best way to proceed, without first having that discussion.

@Akasurde @eqrx As the other members of the Executor team, your input on this would be appreciated.

misslicked, sorry

[eqrx]

Yeah, would agree that having a discussion about this would be best

[eqrx]

I would agree with @Shrews and remove this file altogether since it is not used

[Akasurde]

I agree with @Shrews.