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.

Sign up for GitHub

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

Jump to bottom

chore(ci): extend external contribution to all pr workflows #1985

Merged
merged 2 commits into from
Jan 23, 2025
Merged

chore(ci): extend external contribution to all pr workflows #1985

merged 2 commits into from
Jan 23, 2025

Conversation

soonum
Copy link
Contributor

@soonum soonum commented Jan 17, 2025 •
edited
Loading

User permission checking is done after the should-run, when there is such step, rather than before it.
This way, only workflows that should run would fail id triggering actor is not allowed to launch it.
Thus a repository maintainer would have to re-run only a handful of jobs that would effectively run afterward.

Resolves: zama-ai/tfhe-rs-internal#890

@soonum
chore(ci): use limited access token to checkout repositories
3cfddb6 
This approach allows checkout public and private repository, like
Slab, without to worry too much about secret leakage under certain
circumstances (e.g. under pull request from forks).
The token has just read access on selected repositories.
@soonum soonum added the ci label Jan 17, 2025
@soonum soonum requested a review from IceTDrinker January 17, 2025 09:53
@soonum soonum self-assigned this Jan 17, 2025
@cla-bot cla-bot bot added the cla-signed label Jan 17, 2025
@soonum soonum force-pushed the dt/ci/extend_external_contrib branch 2 times, most recently from a1ef455 to 18d82d6 Compare January 17, 2025 12:59
@soonum soonum requested a review from nsarlin-zama January 17, 2025 13:17
@soonum soonum force-pushed the dt/ci/extend_external_contrib branch from 18d82d6 to c863941 Compare January 17, 2025 15:17
@soonum soonum requested a review from nsarlin-zama January 17, 2025 15:17
@IceTDrinker
Copy link
Member

@soonum lint and checks not happy ?

@soonum
Copy link
Contributor Author

soonum commented Jan 20, 2025

@soonum lint and checks not happy ?

Ok, I see what's going on here. I need to remove the dynamic referencing of workflow calls. <my_workflow>@${{ github.sha }} syntax induce a failure during checking because the version is not pinned.
The solution is: remove the reference, merge this PR, then create a new PR with a SHA from main for these workflows to correctly pin the action.
This would make any change to the two workflows a bit more subtle to test but this is the only solution I can think of to harden security.

@IceTDrinker
Copy link
Member

@soonum lint and checks not happy ?

Ok, I see what's going on here. I need to remove the dynamic referencing of workflow calls. <my_workflow>@${{ github.sha }} syntax induce a failure during checking because the version is not pinned. The solution is: remove the reference, merge this PR, then create a new PR with a SHA from main for these workflows to correctly pin the action. This would make any change to the two workflows a bit more subtle to test but this is the only solution I can think of to harden security.

there is a whitelist for the dep pinning

@soonum soonum force-pushed the dt/ci/extend_external_contrib branch from c863941 to 21fde64 Compare January 20, 2025 09:17
nsarlin-zama
nsarlin-zama approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@nsarlin-zama nsarlin-zama left a comment

Choose a reason for hiding this comment

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

it looks good to me, thanks!

@soonum soonum force-pushed the dt/ci/extend_external_contrib branch from 21fde64 to f41b9b6 Compare January 20, 2025 15:17
@zama-bot zama-bot removed the approved label Jan 20, 2025
@soonum soonum force-pushed the dt/ci/extend_external_contrib branch from f41b9b6 to 917bfb9 Compare January 20, 2025 16:16
@soonum soonum requested a review from nsarlin-zama January 21, 2025 08:26
@soonum soonum force-pushed the dt/ci/extend_external_contrib branch from 917bfb9 to 2958b15 Compare January 22, 2025 14:48
@zama-bot zama-bot removed the approved label Jan 22, 2025
@soonum
chore(ci): extend external contribution to all pr workflows
2f80782 
User permission checking is done after the should-run, when there
is such step, rather than before it. This way, only workflows that
should run would fail id triggering actor is not allowed to launch
it. Thus a repository maintainer would have to re-run only a
handful of jobs that would effectively run afterward
(i.e relevant code has changed and setup-instance would be called).
@soonum soonum force-pushed the dt/ci/extend_external_contrib branch from 2958b15 to 2f80782 Compare January 22, 2025 17:49
@soonum soonum requested a review from nsarlin-zama January 22, 2025 17:50
nsarlin-zama
nsarlin-zama approved these changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@nsarlin-zama nsarlin-zama left a comment

Choose a reason for hiding this comment

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

Looks good to me, good job!

@soonum soonum merged commit 1a3b2d7 into main Jan 23, 2025
147 checks passed
@soonum soonum deleted the dt/ci/extend_external_contrib branch January 23, 2025 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nsarlin-zama nsarlin-zama nsarlin-zama approved these changes

@IceTDrinker IceTDrinker Awaiting requested review from IceTDrinker

Assignees

@soonum soonum

Labels
approved ci cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@soonum @IceTDrinker @nsarlin-zama @zama-bot