Use flake8 (via pantsbuild) by cognifloyd · Pull Request #5776 · StackStorm/st2

cognifloyd · 2022-10-14T21:18:27Z

Background

This is another part of introducing pants, as discussed in the TSC Meetings on 12 July 2022, 02 Aug 2022, 06 Sept 2022, and 04 Oct 2022. Pants has fine-grained per-file caching of results for lint, fmt (like black), test, etc. It also has lockfiles that work well for monorepos that have multiple python packages. With these lockfiles CI should not break when any of our dependencies or our transitive dependencies release new versions, because CI will continue to use the locked version until we explicitly relock with updates.

To keep PRs as manageable/reviewable as possible, introducing pants will take a series of PRs. I do not know yet how many PRs; I will break this up into logical steps with these goals:

introduce pants to the st2 repo, and
teach some (but not all) TSC members about pants step-by-step.

Other pants PRs include:

Overview of this PR

This configures pants to use flake8 when running the lint goal.

I added our st2flake8 plugin to [flake8].extra_requirements (in pants.toml) so that our copyright check will work as it does now. Eventually, I would like to switch to pants' regex-lint so we have less custom code to maintain. For now, this sticks with how we're using flake8 now.

By default, newer versions of pants will use flake8>=5.0.4,<5.1 (see pantsbuild/pants#17226), but our plugin does not support flake8 5. Since we need to use something older, we also need to pin importlib-metadata, which older versions of flake8 do not pin appropriately. We do not currently pin the version of flake8, so I went with flake8==4.0.1 because that is what is getting installed in our CI workflows now.

Pants uses a lockfile for each tool it uses to ensure we get consistent results. Since I changed [flake8].extra_requirements in pants.toml we also need to regenerate the flake8 lockfile. Once we drop our custom flake8 requirements, we can probably just reuse the <default> lockfile for flake8 distributed with pants.

As described in #5774, I'm putting lockfiles under the lockfiles/ directory. I'm also not using an extension on the file. Pants does not care about extensions, so we can adopt our own convention on naming the lockfiles. For now, I skipped the extension, but we could easily use .lock or something similar.

NOTE: it is not safe to manually change the lockfiles. If we need to change any dependencies (including transitive deps), we need to use pants to regenerate the applicable lockfiles. Therefore, 349 lines of this change are auto-generated - you can review them for sanity, but we should not change them manually.

I also added a few skip_flake8=True entries to BUILD files where we aren't currently running flake8 on them. We can address any flake8-identified issues in follow-up PRs.

Relevant Pants documentation

./pants generate-lockfiles goal
./pants lint goal
Python Overview
- Python Linters and Formatters
- flake8 backend
source targets:
- python_sources
- python_source
Video about how pants facilitates "Incrementally Adopting New Tools"

Things you can do with pantsbuild

I needed to generate the lockfiles/flake8 lockfile, which I did with this (the "scheduler" info messages occur whenever pantsd is starting up in the background, so we can ignore those.):

$ ./pants generate-lockfiles --resolve=flake8  
15:25:18.68 [INFO] Initializing scheduler...
15:25:19.01 [INFO] Scheduler initialized.
15:25:46.34 [INFO] Completed: Generate lockfile for flake8
15:25:46.35 [INFO] Wrote lockfile for the resolve `flake8` to lockfiles/flake8

You can run ./pants lint :: to see if flake8 finds any issues (the GHA Lint workflow runs this as well).

$ ./pants lint ::
16:16:37.91 [INFO] Completed: Lint with Shellcheck - shellcheck succeeded.
16:16:43.77 [INFO] Completed: Building flake8.pex from lockfiles/flake8
16:16:45.53 [INFO] Completed: Lint with Flake8 - flake8 succeeded.
16:16:46.40 [INFO] Completed: Lint with Flake8 - flake8 succeeded.
16:16:47.37 [INFO] Completed: Lint with Flake8 - flake8 succeeded.
16:16:48.11 [INFO] Completed: Lint with Flake8 - flake8 succeeded.
16:16:52.67 [INFO] Completed: Lint with Flake8 - flake8 succeeded.
16:16:53.59 [INFO] Completed: Lint with Flake8 - flake8 succeeded.
16:16:54.02 [INFO] Completed: Lint with Flake8 - flake8 succeeded.
16:16:58.38 [INFO] Completed: Lint with Flake8 - flake8 succeeded.
16:17:02.15 [INFO] Completed: Lint with Flake8 - flake8 succeeded.

✓ flake8 succeeded.
✓ shellcheck succeeded.

amanda11

LGTM

rush-skills · 2022-10-17T14:53:06Z

st2actions/bin/BUILD

@@ -1,5 +1,6 @@
 python_sources(
    sources=["st2*"],
+    skip_flake8=True,


Why do we enable skip_flake8 in these?

I skipped the bin files because the Makefile does not run flake8 on them, so there are a variety of violations. Here's what it looks like when I comment out this line in st2common/bin/BUILD:

jafloyd@MB-C02XKFC0JG5H st2 % ./pants lint --only=flake8 :: 10:03:43.98 [INFO] Completed: Lint with Flake8 - flake8 succeeded. 10:03:43.98 [INFO] Completed: Lint with Flake8 - flake8 succeeded. 10:03:43.98 [INFO] Completed: Lint with Flake8 - flake8 succeeded. 10:03:43.98 [INFO] Completed: Lint with Flake8 - flake8 succeeded. 10:03:43.98 [INFO] Completed: Lint with Flake8 - flake8 succeeded. 10:03:43.98 [INFO] Completed: Lint with Flake8 - flake8 succeeded. 10:03:43.98 [INFO] Completed: Lint with Flake8 - flake8 succeeded. 10:03:43.98 [INFO] Completed: Lint with Flake8 - flake8 succeeded. 10:03:46.77 [ERROR] Completed: Lint with Flake8 - flake8 failed (exit code 1). st2common/bin/st2-bootstrap-rmq:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-bootstrap-rmq:1:2: C801 Copyright notice not present. st2common/bin/st2-cleanup-db:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-cleanup-db:1:2: C801 Copyright notice not present. st2common/bin/st2-generate-api-spec:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-generate-api-spec:1:2: C801 Copyright notice not present. st2common/bin/st2-generate-schemas:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-generate-symmetric-crypto-key:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-generate-symmetric-crypto-key:1:2: C801 Copyright notice not present. st2common/bin/st2-pack-download:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-pack-download:1:2: C801 Copyright notice not present. st2common/bin/st2-pack-install:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-pack-install:1:2: C801 Copyright notice not present. st2common/bin/st2-pack-setup-virtualenv:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-pack-setup-virtualenv:1:2: C801 Copyright notice not present. st2common/bin/st2-purge-executions:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-purge-executions:1:2: C801 Copyright notice not present. st2common/bin/st2-purge-rule-enforcements:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-purge-rule-enforcements:1:2: C801 Copyright notice not present. st2common/bin/st2-purge-task-executions:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-purge-task-executions:1:2: C801 Copyright notice not present. st2common/bin/st2-purge-tokens:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-purge-tokens:1:2: C801 Copyright notice not present. st2common/bin/st2-purge-traces:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-purge-traces:1:2: C801 Copyright notice not present. st2common/bin/st2-purge-trigger-instances:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-purge-trigger-instances:1:2: C801 Copyright notice not present. st2common/bin/st2-purge-workflows:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-purge-workflows:1:2: C801 Copyright notice not present. st2common/bin/st2-register-content:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-register-content:1:2: C801 Copyright notice not present. st2common/bin/st2-track-result:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-track-result:1:2: C801 Copyright notice not present. st2common/bin/st2-track-result:18:1: F401 'sys' imported but unused st2common/bin/st2-validate-api-spec:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-validate-api-spec:1:2: C801 Copyright notice not present. st2common/bin/st2-validate-pack:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-validate-pack:24:1: F401 'inspect' imported but unused st2common/bin/st2-validate-pack:25:1: F401 'json' imported but unused st2common/bin/st2-validate-pack:28:1: F401 'yaml' imported but unused st2common/bin/st2-validate-pack-config:1:2: L102 Apache 2.0 license header not found st2common/bin/st2-validate-pack-config:1:2: C801 Copyright notice not present. ✕ flake8 failed.

we can remove skip_flake8 and fix the issues in a follow-up PR (probably a great first contributor PR)

rush-skills

LGTM overall

pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Oct 14, 2022

cognifloyd self-assigned this Oct 14, 2022

cognifloyd added external dependency infrastructure: ci/cd pantsbuild labels Oct 14, 2022

cognifloyd added this to the pants milestone Oct 14, 2022

cognifloyd mentioned this pull request Oct 14, 2022

Use black (via pantsbuild) #5774

Merged

cognifloyd changed the title ~~Pants flake8~~ Use flake8 (via pantsbuild) Oct 14, 2022

cognifloyd mentioned this pull request Oct 15, 2022

Use bandit (via pantsbuild) #5777

Merged

cognifloyd requested review from a team, amanda11, arm4b, nzlosh, rush-skills and winem October 15, 2022 01:57

cognifloyd mentioned this pull request Oct 15, 2022

Use regex-lint to provide circular dependency checking in pants #5778

Merged

amanda11 approved these changes Oct 17, 2022

View reviewed changes

rush-skills reviewed Oct 17, 2022

View reviewed changes

rush-skills approved these changes Oct 17, 2022

View reviewed changes

cognifloyd added 5 commits October 17, 2022 09:59

pants: add flake8 lint backend

13af76b

update changelog entry

f78ace6

pants: add pex lockfile for flake8

ad7a4d5

mention flake8 in GHA lint workflow

fff10ae

use a .lock extension on lockfiles

9a9dc29

cognifloyd force-pushed the pants-flake8 branch from 8679bb8 to 9a9dc29 Compare October 17, 2022 15:01

cognifloyd enabled auto-merge October 17, 2022 15:01

cognifloyd merged commit aaa3b44 into master Oct 17, 2022

cognifloyd deleted the pants-flake8 branch October 17, 2022 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use flake8 (via pantsbuild)#5776

Use flake8 (via pantsbuild)#5776
cognifloyd merged 5 commits intomasterfrom
pants-flake8

cognifloyd commented Oct 14, 2022 •

edited

Loading

Uh oh!

amanda11 left a comment

Uh oh!

rush-skills Oct 17, 2022

Uh oh!

cognifloyd Oct 17, 2022

Uh oh!

cognifloyd Oct 17, 2022

Uh oh!

rush-skills left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

cognifloyd commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Overview of this PR

Relevant Pants documentation

Things you can do with pantsbuild

Uh oh!

amanda11 left a comment

Choose a reason for hiding this comment

Uh oh!

rush-skills Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

cognifloyd Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

cognifloyd Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

rush-skills left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cognifloyd commented Oct 14, 2022 •

edited

Loading