Merged
Conversation
837fb1c to
356fdb0
Compare
This was referenced Oct 15, 2022
amanda11
approved these changes
Oct 17, 2022
Contributor
amanda11
left a comment
There was a problem hiding this comment.
LGTM - although the vote seems to be going for .lock files...
af1aff0 to
53d4e46
Compare
53d4e46 to
d7832e6
Compare
We do not want to have pants or git complaining about changes in the git submodule as those changes would require a separate PR process.
cognifloyd
commented
Oct 17, 2022
Comment on lines
+26
to
27
| # do not fmt across git submodule boundary | ||
| skip_black=True, |
Member
Author
There was a problem hiding this comment.
This was requested in #5774 (comment)
I missed adding it before merging, so I'm adding it here.
Eric-Arellano
approved these changes
Oct 17, 2022
Eric-Arellano
left a comment
There was a problem hiding this comment.
Bandit is one of my favorite tools. Nice :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
pantsto the st2 repo, andpantsstep-by-step.Other pants PRs include:
pants_ignoreand bump pants to v2.14.0rc1 #5733BUILDfiles with./pants tailor ::#5738Overview of this PR
This configures pants to use
banditwhen running thelintgoal.By default, pants uses
bandit>=1.7.0,<1.8, but I told it to use the version we have it pinned to:bandit==1.7.0:st2/test-requirements.txt
Line 10 in 83904f6
Pants uses a lockfile for each tool it uses to ensure we get consistent results. Since I changed
[bandit].versioninpants.toml, pants errors, saying that the lockfile (in this case the<default>lockfile distributed with pants) is out-of-date. Note how the message is very helpful in explaining exactly what has to happen to fix this: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.lockor 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, 481 lines of this change are auto-generated - you can review them for sanity, but we should not change them manually.
Unlike
black(#5774) andflake8(#5776), we do not need to addskip_bandit=Truein any of the BUILD files.I used
[bandit].argsto reuse the bandit settings we already have in the Makefile:st2/Makefile
Line 556 in 94d0798
st2/pants.toml
Lines 88 to 93 in 837fb1c
I had to look up what each of the args meant, so I documented the
-lllarg in a comment, and used--excludeinstead of the shorter-xso that it's easier to tell what it's doing. Also, I added the--quietarg because bandit has a very verbose success message--this was recommended in the pants docs.Relevant Pants documentation
./pants generate-lockfilesgoal./pants lintgoalpython_sourcespython_sourceThings you can do with pantsbuild
I needed to generate the
lockfiles/banditlockfile, which I did with this (the "scheduler" info messages occur wheneverpantsdis starting up in the background, so we can ignore those.):You can run
./pants lint ::to see if bandit finds any issues (the GHA Lint workflow runs this as well). You can run only one of the linters with the helpful--onlyarg like this:The
--onlyflag is "scoped" to the lint goal. So, these are equivalent if you wanted to specify that flag in a different order:./pants lint --only=bandit ::./pants --lint-only=bandit lint ::./pants lint :: --lint-only=banditnote: that I had to add
lintin the arg name to use a different order