semaphore-protocol / semaphore

A zero-knowledge protocol for anonymous interactions.
https://semaphore.pse.dev
MIT License
883 stars 193 forks source link

chore: optimize pull-requests workflow #765

Closed sripwoud closed 3 months ago

sripwoud commented 3 months ago

This PR modifies the pull requests GitHub workflow.
Relevant checks will be run conditionally only if the related src files are changed.

It also includes an additional build checks on apps/docs (Closes #756)

Test Plan

Observe that all the workflow steps unrelated to this PR were skipped.
Indeed this PR only modifies a yml files in .github, observe that it doesn't belong to any glob patterns part of the list of files we want to track. So as expected for this PR:

Verify only relevant jobs steps are run when corresponding files are changed (steps aren't skipped when they shouldn't): :heavy_check_mark: see https://github.com/semaphore-protocol/semaphore/pull/765#issuecomment-2097836509

Related Issue(s)

756

Next

Mirror these changes in the production workflow once approach in this PR is agreed on.

Checklist

sripwoud commented 3 months ago

@baumstern

sripwoud commented 3 months ago

Before I open a PR, I like to make sure everything's checked out by testing different sections—like circuits and contracts—with some dummy changes in my forked repo. Typically, I'll switch the CI trigger from something like pull_request trigger to push trigger and check if the CI runs succeed for different scenarios. Once everything looks good, I go ahead and open the PR.

yes that's more or less what I usually do too. :wink: I also like to open PRs early to get feedbacks early too and avoiding putting to much work into something that may go in the wrong direction.

https://nektosact.com/ this is really nice, I didn't know! Will definitely use it in the future!

sripwoud commented 3 months ago

I had a few mistakes in the workflow files that I fixed in the last commits. These workflows triggered on branches with fake changes demonstrate the expected behavior (what should be skipped is skipped, what should not be skipped is not):

sripwoud commented 3 months ago

I'd say another review should be ok to merge.

@baumstern how does it look?

sripwoud commented 3 months ago

Let's merge this before #757

vplasencia commented 3 months ago

Thanks @sripwoud for the great work.