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

Skip signing if key pass isn't available #377

Merged

Conversation

mikecook
Copy link
Contributor

@mikecook mikecook commented Dec 18, 2021

Environmental secrets are not shared to workflows run from forks without extra work by Maintainers.

The existing build workflow uses a secret to sign windows exe's and is breaking for all external PRs.

See: #353 (comment)

@mikecook mikecook closed this Dec 18, 2021
@mikecook mikecook reopened this Dec 18, 2021
Copy link
Contributor

@alerque alerque left a comment

Choose a reason for hiding this comment

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

I think build's should build for PRs. This is useful for internal development (although whether the developer uses this workflow or not is up to them) to confirm that CI works before a release is tagged. If your builds only run on tagged releases chances are your releases will be broken the first time around when it comes time for them.

Also GitHub has some plumbing to block CI runs on PRs for first time contributors, and for returning contributors having builds in place serves a purpose.

I suggest instead fixing the builds so the signing step is optional and only runs when the source repo is this repo and run everything else as normal.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@mikecook
Copy link
Contributor Author

I think build's should build for PRs. This is useful for internal development (although whether the developer uses this workflow or not is up to them) to confirm that CI works before a release is tagged. If your builds only run on tagged releases chances are your releases will be broken the first time around when it comes time for them.

Reasonable, but it does depend on developer workflow, and that hasn't been established yet.

Also GitHub has some plumbing to block CI runs on PRs for first time contributors, and for returning contributors having builds in place serves a purpose.

That plumbing is already enabled for this repo, but because environment secrets don't get passed it leads to breakage when secrets are involved.

I suggest instead fixing the builds so the signing step is optional and only runs when the source repo is this repo and run everything else as normal.

That's my favorite solution, ...and I just figured out how to do that, revising PR

@mikecook mikecook force-pushed the skip_builds_for_pull_requests branch from b460da9 to 988c2c8 Compare December 18, 2021 10:58
@mikecook mikecook changed the title Don't run build for Pull Requests Don't run build for forked repo Dec 18, 2021
.github/workflows/build.yml Outdated Show resolved Hide resolved
@mikecook mikecook force-pushed the skip_builds_for_pull_requests branch from 988c2c8 to b1a68cf Compare December 18, 2021 12:48
@mikecook mikecook changed the title Don't run build for forked repo Split build/sign into separate jobs Dec 18, 2021
@mikecook mikecook force-pushed the skip_builds_for_pull_requests branch 2 times, most recently from aa55da5 to 64a4857 Compare December 18, 2021 13:28
@mikecook mikecook force-pushed the skip_builds_for_pull_requests branch from 64a4857 to ae5dd6a Compare December 18, 2021 14:59
@mikecook mikecook changed the title Split build/sign into separate jobs Skip signing if key pass isn't available Dec 18, 2021
See https://docs.github.com/en/actions/learn-github-actions/events-that-trigger-workflows#pull-request-events-for-forked-repositories
With the exception of GITHUB_TOKEN, secrets are not passed to the
runner when a workflow is triggered from a forked repository.
@mikecook mikecook force-pushed the skip_builds_for_pull_requests branch from ae5dd6a to 65b3620 Compare December 18, 2021 15:07
@FiloSottile FiloSottile merged commit 08f52cc into FiloSottile:main Dec 18, 2021
@FiloSottile
Copy link
Owner

Thank you!

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.

None yet

3 participants