Skip to content

Improve Github action workflows#7651

Merged
Pierre-Sassoulas merged 10 commits intopylint-dev:mainfrom
cdce8p:fix-ci
Nov 4, 2022
Merged

Improve Github action workflows#7651
Pierre-Sassoulas merged 10 commits intopylint-dev:mainfrom
cdce8p:fix-ci

Conversation

@cdce8p
Copy link
Copy Markdown
Member

@cdce8p cdce8p commented Oct 19, 2022

Description

Improve multiple issues with our Github actions workflows.

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
Copy Markdown

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
Copy Markdown
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 👌

- name: Run pytest
run: |
. venv/bin/activate
pip list | grep 'astroid\|pylint'
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

4 participants