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

Run code quality checks only once in testing workflow #2055

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

pkalita-lbl
Copy link
Contributor

@pkalita-lbl pkalita-lbl commented Apr 5, 2024

Fixes #2054

Summary

  • Moved code quality checks to a new job in main.yaml. This job only runs once and if it successfully completes the test job (with the same permutations as before) will run.
  • Added a call to poetry check to the code quality job. This will fail if our pyproject.toml file has formatting issues or is out of sync with poetry.lock (https://python-poetry.org/docs/cli/#check). This was inspired by the recent attempts to get automatic, periodic updates to the lock file working.
  • Updated Poetry installation and virtual environment caching to be inline with the current best practices suggested by the setup-python action https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#caching-packages

Comments

  • With the way the workflow is currently configured, the tests will not run if the code quality checks fail. This is consistent with the current behavior (except that the quality checks only happen once). But by having them as separate jobs, we now have control over that. If we removed the needs block in the test job, the code quality checks and tests could run in parallel. Something to consider.
  • I really like that the setup-python action now has built-in support for Poetry virtual environment caching. It's nice to let someone else get that right for us! If it seems to work well here we should roll that out to workflows in our other repos.

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.71%. Comparing base (68aa907) to head (85c693b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2055   +/-   ##
=======================================
  Coverage   79.71%   79.71%           
=======================================
  Files         107      107           
  Lines       11967    11967           
  Branches     3418     3418           
=======================================
  Hits         9540     9540           
  Misses       1853     1853           
  Partials      574      574           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pkalita-lbl pkalita-lbl changed the title WIP: Move code quality checks to separate job Run code quality checks only once in testing workflow Apr 5, 2024
@pkalita-lbl pkalita-lbl marked this pull request as ready for review April 5, 2024 18:25
@sierra-moxon
Copy link
Member

Summary

  • Moved code quality checks to a new job in main.yaml. This job only runs once and if it successfully completes the test job (with the same permutations as before) will run.
    ...
  • With the way the workflow is currently configured, the tests will not run if the code quality checks fail. This is consistent with the current behavior (except that the quality checks only happen once). But by having them as separate jobs, we now have control over that. If we removed the needs block in the test job, the code quality checks and tests could run in parallel. Something to consider.

and we want two jobs stanzas in one action file vs. two action files each with their own single job, to take advantage of the chained execution here? (and/or share setup)

@pkalita-lbl
Copy link
Contributor Author

Right, having them as two jobs in one workflow allows you to run one conditionally based on the result of the other. If we didn't want/need that then there wouldn't be much of a difference between two jobs in one workflow and two workflows.

@pkalita-lbl pkalita-lbl merged commit 5369b6c into main Apr 5, 2024
14 checks passed
@pkalita-lbl pkalita-lbl deleted the issue-2054-workflow-cleanup branch April 5, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not run code quality checks on every testing matrix permutation
2 participants