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

chore: add pre-commit downstream workflows #386

Merged

Conversation

maxArturo
Copy link
Contributor

Overview

  • If there's a pre-commit update:
    • "require" and run the lint and test workflows, avoiding any direct recursive calls to GH actions

Description

Based on our previous conversation I looked at how to get this running with dependabot, but I quickly realized it wouldn't work as expected.

It would only run when a pip dependency changed, so effectively it wouldn't run the specific code that the current GH action runs with a daily frequency.

Also, using dependabot with pre-commit is very low on the GH team's priority.

So I decided to "unfurl" the needed downstream actions as direct dependencies on the "pre-commit" job, by "requiring" them and conditionally running linting and testing if there's a pre-commit update. This works because we can see if the pr was created or updated in the outputs of the create-pr action.

PROOF:

You can see the successful run and then the lint/test actions being triggered afterwards.

Additional Details

The one thing to keep in mind is if you want something else to conditionally run based on pre-commit update, you have to add it here as a direct downstream dependency. That's it!

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@clavedeluna
Copy link
Contributor

You can see the successful run and then the lint/test actions being triggered afterwards.

But the unit tests did not run ?

@maxArturo
Copy link
Contributor Author

maxArturo commented Mar 18, 2024 via email

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.47%. Comparing base (e33e8ea) to head (053cebd).
Report is 119 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
- Coverage   96.60%   94.47%   -2.13%     
==========================================
  Files         101      126      +25     
  Lines        4275     5543    +1268     
==========================================
+ Hits         4130     5237    +1107     
- Misses        145      306     +161     

see 8 files with indirect coverage changes

@clavedeluna
Copy link
Contributor

IFF integration tests fail it's because of timeout, fixing it here #387

Copy link
Contributor

@clavedeluna clavedeluna 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 a reasonable approach to solve for the minor problem of having to manually pull and force push that auto-update commit.

Do want @drdavella to give this approval before merge though.

@drdavella
Copy link
Member

@maxArturo thanks again for the contribution. I think this is pretty cool and it looks like it works.

I recently discovered an action for checking diffs and I wonder whether it would help simplify this action at all? It's not a blocker but I'm curious looking ahead: https://github.com/technote-space/get-diff-action

I also wonder whether we can use a similar approach to make the PyPI release conditional on test execution. It's hard to do that right now because you can't filter a workflow_run action on a specific tag (only on a branch). Anyway, just an idle thought, maybe it doesn't make much sense.

@drdavella drdavella added this pull request to the merge queue Mar 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2024
@maxArturo
Copy link
Contributor Author

Hi @drdavella my pleasure!

I recently discovered an action for checking diffs and I wonder whether it would help simplify this action at all? It's not a blocker but I'm curious looking ahead: https://github.com/technote-space/get-diff-action

Oh that's neat! Looks like it's very full featured. I do worry about the fact that it's archived/frozen, and it might be heavy-handed for this use case. I think the git diff work is already being done by peter-evans/create-pull-request: if there's no changes to the repo I believe it won't create/update any PR's, which is why this is the conditional:

       if: |
          (steps.pr-create.outputs.pull-request-operation == 'created' ||
          steps.pr-create.outputs.pull-request-operation == 'updated') &&
          steps.pr-create.outputs.pull-request-number

If we ever need a more advanced filter criteria (e.g a subset of files/folders) to conditionally execute, I'd recommend running a "straight up" custom git diff command and avoid pulling in an entire node/NPM project that will do almost the same! 😄

I also wonder whether we can use a similar approach to make the PyPI release conditional on test execution. It's hard to do that right now because you can't filter a workflow_run action on a specific tag (only on a branch). Anyway, just an idle thought, maybe it doesn't make much sense.

Could be! Looks like the releases are handled manually, so you could just call a uses from this line and include whatever workflows you'd like to run before the pypi release happens.

I haven't had to do this before, but there's a good opportunity keep things DRY and group your code sanity GH workflows in an "omnibus" yml file. Then, you'd uses that in all the spots where you want a gate (at this point, here and on release it seems). There seems to be a balance between autotriggers (on: push, etc) and "manual" calling, but only the team members know best which fits where.

@drdavella drdavella added this pull request to the merge queue Mar 20, 2024
Merged via the queue into pixee:main with commit ad9c600 Mar 20, 2024
14 checks passed
clavedeluna added a commit that referenced this pull request Mar 25, 2024
github-merge-queue bot pushed a commit that referenced this pull request Mar 25, 2024
Revert "chore: add pre-commit downstream workflows (#386)"

This reverts commit ad9c600.
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.

3 participants