Skip to content
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

Cache and store primer packages via the default branch #6703

Merged
merged 6 commits into from
May 27, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
🔨 Refactoring

Description

Ref. #5364.

This is in preparation of a full fledged primer which can warn us about false positives and negatives.

This adds:

  1. A workflow triggered on every push to main that caches and downloads all packages in tests/primer/packages_to_prime.json.
  2. A workflow triggered on all relevant PRs that searches for the last successful run on main and gets its cache from there.

The cache is managed by what I call the commitstring, which is just a concatenation of all commits that the packages to prime are on.

Because this needs to be merged into main to work you can see a working example here:
Job on main:
https://github.com/DanielNoord/pylint/runs/6606171195?check_suite_focus=true
PR that gets triggered after that push:
DanielNoord#145
Successful job on the PR:
https://github.com/DanielNoord/pylint/runs/6606228882?check_suite_focus=true

You can look at the restore cache jobs to see that this works.

In itself this PR doesn't really add much, but the extra run time is about 60 seconds for both jobs together and only about 20 seconds for a PR. Since the workflow for primer depends on stuff being already merged on main I'd like to do some preparation PRs to build on like this.

@coveralls
Copy link

coveralls commented May 26, 2022

Pull Request Test Coverage Report for Build 2396032378

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.42%

Totals Coverage Status
Change from base Build 2386903518: 0.0%
Covered Lines: 16229
Relevant Lines: 17008

💛 - Coveralls

@Pierre-Sassoulas
Copy link
Member

Should we set the commit we're analyzing for each repositories ? I.e. we set the sha for each package to prime. This way we can't have a new message that appears simply because something was added on the primed repo. We could upgrade the primer repo to the latest version before release so we're still somewhat up to date.

@DanielNoord
Copy link
Collaborator Author

Should we set the commit we're analyzing for each repositories ? I.e. we set the sha for each package to prime. This way we can't have a new message that appears simply because something was added on the primed repo. We could upgrade the primer repo to the latest version before release so we're still somewhat up to date.

I don't think so.

The idea is to run main over a set of packages on specific commit. That commit is the latest commit of each package on their default branch at the time of the run. Then main records those commits as the commitstring.
A PR then gets the commitstring and retrieves the cache created by main and validates it. That way main and PR are both on the same commits of the packages (so even if the package adds new commits between the main and the PR run, those don't affect us).

Lastly, we run PR over the packages and compare its output with main. Notifying of any added messages or any removed messages. In that way it is similar to the current primer: we don't really care about which messages are emitted when with --enable=all, --enable-all-extensions. We only care about the difference between main and PR.

.github/workflows/primer_run_main.yaml Show resolved Hide resolved
id: cache-projects
uses: actions/cache@v3
with:
path: .pylint_primer_tests/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a .gitignore to this directory to ensure it always exists? I'm thinking for local testing etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a .gitkeep ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

Comment on lines 85 to 87
if commit_string:
with open("commit_string.txt", "w", encoding="utf-8") as f:
f.write(commit_string)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should add to the project's .gitignore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed this to go into the primer directory instead of writing to root.

.github/workflows/primer_run_main.yaml Outdated Show resolved Hide resolved
.github/workflows/primer_run_pr.yaml Outdated Show resolved Hide resolved
tests/primer/primer_tool.py Outdated Show resolved Hide resolved
tests/primer/primer_tool.py Outdated Show resolved Hide resolved
DanielNoord and others added 3 commits May 27, 2022 12:26
Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
@DanielNoord
Copy link
Collaborator Author

Test run on my fork main:
https://github.com/DanielNoord/pylint/runs/6624159814
And subsequent run of the PR workflow on a PR:
https://github.com/DanielNoord/pylint/runs/6624174015

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty great ! We'll have even more confidence when releasing.

@DanielNoord DanielNoord merged commit a6a23ec into pylint-dev:main May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants