-
-
Notifications
You must be signed in to change notification settings - Fork 751
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
Add GHA workflow to lint with pantsbuild #5758
Conversation
18d44d5
to
109df1f
Compare
.github/workflows/lint.yaml
Outdated
schedule: | ||
# run every night at midnight | ||
- cron: '0 0 * * *' |
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 this?
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 copied that from our other workflows which we run every day. We did that in part to catch issues with transitive deps, which will not be a significant issue once we've got the lockfiles in. So, I suppose it is unlikely for linters to fail from one day to another, so maybe we don't have to be quite so aggressive here. 🤔
What do others think? Drop this? Comment it out for now?
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.
which will not be a significant issue once we've got the lockfiles in.
Note that you already are using "tool lockfiles". Pants ships them by default. So your installation of tools like Flake8 and Pylint will be deterministic. It is only installs of your own user code that need to manually set up lockfiles, since we can't ship them without knowing before what your code is.
So running this is cron is likely a waste of compute resources (and trees)
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 commented it out.
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, added some comments below
./scripts/github/install-apt-packages-use-cache.sh | ||
|
||
- name: Initialize Pants and its GHA caches | ||
uses: pantsbuild/actions/init-pants@c0ce05ee4ba288bb2a729a2b77294e9cb6ab66f7 |
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.
can't we point to master or latest release or something here
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.
This repo does not yet have tags we can point to. If there are changes, we might need to adjust how we use the action, app pinning to a particular version seemed the best way to go.
When we choose another approach (eg when there are tags available), then we'll need to change the pants BUILD validation workflow add well:
st2/.github/workflows/pants.yaml
Line 33 in 989e4ec
uses: pantsbuild/actions/init-pants@c0ce05ee4ba288bb2a729a2b77294e9cb6ab66f7 |
gha-cache-key: cache0 | ||
# This hash should include all of our lockfiles so that the pip/pex caches | ||
# get invalidated on any transitive dependency update. | ||
named-caches-hash: ${{ hashFiles('requirements.txt') }} |
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.
is the root level requirements.txt
enough in this scenario?
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.
Not entirely. I plan to change this to use the lockfiles instead once the lockfiles are available.
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 - once the other queries on the PR have been addressed.
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
We begin to add linters in #5751. This adds a GHA workflow to automatically run the linters. This workflow copies a few pants tasks from the pants validation workflow added in #5732 and the apt tasks from our current CI workflow.
Until one of the linter backends is registered,
./pants lint ::
will just return with success because there is nothing to do.When we add python linters (black, flake8, etc) we will need to expand this workflow to handle a matrix of the various python versions. For now, this is sufficient for running shellcheck (in #5751) and a few other linters.
Relevant Pants documentation
./pants lint
goalThings you can do with pantsbuild
You can run
./pants lint ::
but it doesn't do anything yet. So, output looks like this: