Skip to content

SPEC 8 signed commits clarification #380

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

Closed
lucascolley opened this issue Mar 28, 2025 · 9 comments · Fixed by #387
Closed

SPEC 8 signed commits clarification #380

lucascolley opened this issue Mar 28, 2025 · 9 comments · Fixed by #387

Comments

@lucascolley
Copy link
Contributor

lucascolley commented Mar 28, 2025

I am looking into addressing the following sentence of SPEC 8 at data-apis/array-api-extra#166:

It is also strongly recommended that the repository requires signed commits so that each release corresponds to a verified commit.

Since I don't want to require signed commits from every contributor, the easiest way to do this seems to be to require signed commits just on the release branch. The GitHub docs at https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-signed-commits say:

You can also merge signed and verified commits into the branch using a pull request. However, you cannot squash and merge a pull request into the branch on GitHub unless you are the author of the pull request. You can squash and merge pull requests locally.

But that doesn't really answer these questions: can I merge unsigned commits into a protected branch as a signed merge commit? Would such a merge have to be a squash? If so, can I do this from the GitHub web UI, or only locally?

cc @matthewfeickert

@tupui
Copy link
Member

tupui commented Mar 28, 2025

When you merge commits from the UI they become signed commits.

Signing the commits is more a way to prove that a specific person work on some code. Anyone can otherwise change the name and email in the commit and there is no way to prove anything. Signing gives that guarantee.

@lucascolley
Copy link
Contributor Author

When you merge commits from the UI they become signed commits.

Thanks! So does that mean I could do the following:

  • require signed commits on the main branch
  • merge (either via a merge commit or a squash, and through the GitHub web UI or locally) PRs with unsigned commits to main

@tupui
Copy link
Member

tupui commented Mar 28, 2025

Yes I think that works 👍

@lucascolley
Copy link
Contributor Author

Okay, sounds good to me if that works. @rgommers would you be willing to turn on requiring signed commits for array-api-extra's main branch? Then we can give this a try and check that it does work.

@rgommers
Copy link
Contributor

Sure - done now!

@lucascolley
Copy link
Contributor Author

does that mean I could do the following:

  • require signed commits on the main branch
  • merge (either via a merge commit or a squash, and through the GitHub web UI or locally) PRs with unsigned commits to main

Yes I think that works 👍

@tupui it looks like that doesn't work, on data-apis/array-api-extra#221 I see:

Image

@tupui
Copy link
Member

tupui commented Apr 11, 2025

Well that's unfortunate then I guess that's not an option for repos like these.

@lucascolley
Copy link
Contributor Author

I think we might want to amend the SPEC, unless we seriously want to recommend a workflow like the release manager squashing all commits over to a separate release branch locally.

Perhaps replacing it with a recommended policy that release managers sign their commits would be good enough.

Any thoughts @matthewfeickert ?

@tupui
Copy link
Member

tupui commented Apr 11, 2025

We could add a note saying that there are some caveats to use this feature in GitHub. What we still want to recommend is that people sign their commits and yes encouraging specific people like release manager to sign the last commit before a release is a good idea.

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 a pull request may close this issue.

3 participants