-
-
Notifications
You must be signed in to change notification settings - Fork 746
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
Use flake8 (via pantsbuild) #5776
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -1,5 +1,6 @@ | |||
python_sources( | |||
sources=["st2*"], | |||
skip_flake8=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we enable skip_flake8
in these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove skip_flake8 and fix the issues in a follow-up PR (probably a great first contributor PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
8679bb8
to
9a9dc29
Compare
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:
pants
to the st2 repo, andpants
step-by-step.Other pants PRs include:
pants_ignore
and bump pants to v2.14.0rc1 #5733BUILD
files with./pants tailor ::
#5738Overview of this PR
This configures pants to use
flake8
when running thelint
goal.I added our st2flake8 plugin to
[flake8].extra_requirements
(inpants.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 withflake8==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
inpants.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
goalpython_sources
python_source
Things you can do with pantsbuild
I needed to generate the
lockfiles/flake8
lockfile, which I did with this (the "scheduler" info messages occur wheneverpantsd
is starting up in the background, so we can ignore those.):You can run
./pants lint ::
to see if flake8 finds any issues (the GHA Lint workflow runs this as well).