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

Improve Github action workflows #7651

Merged
merged 10 commits into from Nov 4, 2022
Merged

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Oct 19, 2022

Description

Improve multiple issues with our Github actions workflows.

  • Remove restore-keys. Especially on Windows there are issues with reusing old cache entries and trying to install newer versions. Those are just skipped if the old one still satisfies the requirement. It's easy / fast enough to create a whole new environment if something changes.
  • Log astroid and pylint versions to help with debugging if necessary.
  • Reset cache versions.
  • Add check-latest: true for setup-python action. This will prevent cache restore issue when runners use different Python patch versions. https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#check-latest-version
  • Add KEY_PREFIX env variable to more easily identify the key prefix in each workflow.
  • Replace setup.cfg with pyproject.toml for file hash
  • Move comment-hider version in comment after sha. With the latest update, dependabot is now able to update it as well. That does not mean I would recommend replacing the version pins for the Github actions. https://github.blog/changelog/2022-10-31-dependabot-now-updates-comments-in-github-actions-workflows-referencing-action-versions/
  • Add pins for the latest version of setup-node and github-script

Note
With these changes, it's only necessary to bump the primer cache versions when updating astroid.

@cdce8p cdce8p added Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry labels Oct 19, 2022
@coveralls
Copy link

coveralls commented Oct 19, 2022

Pull Request Test Coverage Report for Build 3377667671

  • 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.381%

Totals Coverage Status
Change from base Build 3367355028: 0.0%
Covered Lines: 17241
Relevant Lines: 18076

💛 - Coveralls

@cdce8p cdce8p changed the title Fix Github action workflows Improve Github action workflows Oct 19, 2022
@cdce8p cdce8p force-pushed the fix-ci branch 2 times, most recently from 97637d0 to 4ee878a Compare October 31, 2022 11:02
@cdce8p cdce8p marked this pull request as ready for review November 2, 2022 13:13
@cdce8p cdce8p added the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Nov 2, 2022
@cdce8p cdce8p added this to the 2.15.6 milestone Nov 2, 2022
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.

Thank you, I'll be glad to not have to update the cache all the time 👌

@@ -305,4 +309,5 @@ jobs:
- name: Run pytest
run: |
. venv/bin/activate
pip list | grep 'astroid\|pylint'
Copy link
Member

Choose a reason for hiding this comment

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

Is that a forgotten debug statement or is this voluntary ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a debug statement, but I think a useful one. It can help debug issues when updating astroid and simplify checking if the correct astroid version is used.

@Pierre-Sassoulas
Copy link
Member

I'm wondering if we should fix the primer so they recreate the venv if they can't recover it from cache. This is really an annoying bug it's worth the use of CI imo. Unless there's a real problem with the cache that never work then we need to fix it elsewhere.

@cdce8p
Copy link
Member Author

cdce8p commented Nov 4, 2022

I'm wondering if we should fix the primer so they recreate the venv if they can't recover it from cache. This is really an annoying bug it's worth the use of CI imo. Unless there's a real problem with the cache that never work then we need to fix it elsewhere.

I haven't spend much time looking at it but as I understand it we want to restore a base run from main and if we don't have one to restore, it fails. Maybe @DanielNoord does know something more?

In any case, we can address this separately in another PR.

@cdce8p
Copy link
Member Author

cdce8p commented Nov 4, 2022

Thank you, I'll be glad to not have to update the cache all the time 👌

We still need to update the Primer cache manually, unfortunately. At least for now.

@Pierre-Sassoulas Pierre-Sassoulas merged commit e041d7b into pylint-dev:main Nov 4, 2022
@cdce8p cdce8p deleted the fix-ci branch November 4, 2022 10:25
@DanielNoord
Copy link
Collaborator

Yeah I made the decision to have only main restore the venv because otherwise we need to add additional checks that the PR branch doesn't create a venv that is incompatible with the main venv.

@cdce8p
Copy link
Member Author

cdce8p commented Nov 4, 2022

Yeah I made the decision to have only main restore the venv because otherwise we need to add additional checks that the PR branch doesn't create a venv that is incompatible with the main venv.

With the cache access restrictions, any cache created from the PR branch can only be used for that PR. So it might actually be safe. Furthermore, the cache only includes the venv which isn't modified after the initial install.

I can prepare a PR to show what I had in mind.

https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache

@Pierre-Sassoulas Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Nov 16, 2022
Pierre-Sassoulas pushed a commit that referenced this pull request Nov 16, 2022
* Remove restore keys
* Log pylint + astroid versions
* Reset cache versions
* Add check-latest to setup-python
* Use pyproject.toml for hash
* Update comment-hider version comment
* Pin additional actions
Pierre-Sassoulas pushed a commit that referenced this pull request Nov 17, 2022
* Remove restore keys
* Log pylint + astroid versions
* Reset cache versions
* Add check-latest to setup-python
* Use pyproject.toml for hash
* Update comment-hider version comment
* Pin additional actions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants