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

feat(git): add support for SSH-based commit signing #29550

Merged
merged 15 commits into from
Aug 21, 2024

Conversation

sisp
Copy link
Contributor

@sisp sisp commented Jun 9, 2024

Changes

I've added support for SSH-based commit signing.

Context

Closes #18197.

The implementation includes a workaround for a quirk in ssh-keygen which requires the public SSH key to exist for signing even when the private key is provided directly, which is technically not necessary (see #18197 (comment)).

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@sisp sisp force-pushed the feat/ssh-commit-signing branch 2 times, most recently from b9a65fe to 8920021 Compare June 9, 2024 09:18
@sisp
Copy link
Contributor Author

sisp commented Jun 9, 2024

I'm not sure what to do about the type error. 🤔 Any advice?

@rarkins
Copy link
Collaborator

rarkins commented Jun 9, 2024

Try refactoring the existing part first (separate PR, to be merged first). This is our preferred approach to such changes regardless of any type challenges

@sisp
Copy link
Contributor Author

sisp commented Jun 9, 2024

I think the problem is caused by @types/jest-when: DefinitelyTyped/DefinitelyTyped#68605 (comment)

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

I would prefer to not use jest when and stick to current test dependencies

@sisp
Copy link
Contributor Author

sisp commented Jun 10, 2024

Okay, I noticed that jest-mock-extended also has a calledWith() extension, so I'll try to modify the tests to use it instead.

@sisp sisp force-pushed the feat/ssh-commit-signing branch 2 times, most recently from afe918a to 7b239f2 Compare June 10, 2024 10:16
Copy link
Contributor Author

@sisp sisp left a comment

Choose a reason for hiding this comment

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

I've refactored the tests to use jest-mock-extended instead of jest-when. Rewriting the tests also revealed a bug which I've fixed, too. If the CI checks pass, I think this PR is ready for a proper review.

@sisp sisp force-pushed the feat/ssh-commit-signing branch from 7b239f2 to 16ab23d Compare June 10, 2024 10:24
@viceice viceice self-requested a review June 11, 2024 07:50
lib/util/git/private-key.spec.ts Outdated Show resolved Hide resolved
lib/util/git/private-key.spec.ts Outdated Show resolved Hide resolved
lib/util/git/private-key.spec.ts Outdated Show resolved Hide resolved
lib/util/git/private-key.ts Show resolved Hide resolved
@sisp sisp force-pushed the feat/ssh-commit-signing branch from 39e4a01 to 2f115ad Compare June 30, 2024 09:06
Copy link
Contributor Author

@sisp sisp left a comment

Choose a reason for hiding this comment

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

@viceice I've updated this PR to include the factored out preparatory work from #29875 and addressed your review feedback. WDYT?

@sisp sisp requested a review from viceice July 7, 2024 06:31
lib/util/git/private-key.spec.ts Outdated Show resolved Hide resolved
lib/util/git/private-key.spec.ts Outdated Show resolved Hide resolved
test/util.ts Outdated Show resolved Hide resolved
lib/util/git/private-key.spec.ts Outdated Show resolved Hide resolved
lib/util/git/private-key.ts Outdated Show resolved Hide resolved
lib/util/git/private-key.ts Outdated Show resolved Hide resolved
@sisp sisp force-pushed the feat/ssh-commit-signing branch from 71da848 to 9ee2ebc Compare July 9, 2024 14:10
@sisp
Copy link
Contributor Author

sisp commented Jul 9, 2024

Thanks for the thorough and valuable feedback, @viceice! 🙇 I've addressed your remarks. WDYT? 🙂

@sisp sisp requested a review from viceice July 12, 2024 09:10
@viceice
Copy link
Member

viceice commented Jul 26, 2024

Please don't force-push. Just merge main if required, should only be required on conflicts

@sisp
Copy link
Contributor Author

sisp commented Jul 26, 2024

I didn't force-push to rebase onto the latest state of main but only amended and force-pushed the most recent commit before your subsequent review because it caused the CI pipeline to fail. But if you prefer, I can push new commits for such iterations between reviews.

@viceice
Copy link
Member

viceice commented Jul 26, 2024

yes, please just push new changes. we squash merge PRs, so don't worry about git history

@sisp
Copy link
Contributor Author

sisp commented Jul 26, 2024

I've merged main into here and fixed the temporary directory path like in 1f0ce9a. WDYT, @viceice?

@viceice viceice enabled auto-merge August 19, 2024 15:14
@viceice viceice added the auto:no-done-comments Don't say "Done" or "Please review" - request a review instead label Aug 19, 2024
Copy link
Contributor

Hi there,

You are using done comments which cause a lot of noise/notifications. Instead, please use GitHub's web interface to request another review. Please read our contributing guidelines to reduce noise.

Good luck,

The Renovate team

@rarkins rarkins removed the auto:no-done-comments Don't say "Done" or "Please review" - request a review instead label Aug 21, 2024
@viceice viceice added this pull request to the merge queue Aug 21, 2024
Merged via the queue into renovatebot:main with commit fa8d3ff Aug 21, 2024
39 checks passed
@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 38.46.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sisp sisp deleted the feat/ssh-commit-signing branch August 21, 2024 10:51
@candrews
Copy link
Contributor

I believe this change is causing Renovate to not work with opengpg commit signing.

I'm seeing this error using Renovate 38.46.0:

         "message": "error: invalid value for 'gpg.format': 'opengpg'\nfatal: bad config variable 'gpg.format' in file '.git/config' at line 24\n",
         "stack": "Error: error: invalid value for 'gpg.format': 'opengpg'\nfatal: bad config variable 'gpg.format' in file '.git/config' at line 24\n\n    at Object.action (/usr/local/renovate/node_modules/.pnpm/[email protected]/node_modules/simple-git/src/lib/plugins/error-detection.plugin.ts:42:29)\n    at PluginStore.exec (/usr/local/renovate/node_modules/.pnpm/[email protected]/node_modules/simple-git/src/lib/plugins/plugin-store.ts:54:29)\n    at /usr/local/renovate/node_modules/.pnpm/[email protected]/node_modules/simple-git/src/lib/runners/git-executor-chain.ts:124:42\n    at new Promise (<anonymous>)\n    at GitExecutorChain.handleTaskData (/usr/local/renovate/node_modules/.pnpm/[email protected]/node_modules/simple-git/src/lib/runners/git-executor-chain.ts:121:14)\n    at GitExecutorChain.<anonymous> (/usr/local/renovate/node_modules/.pnpm/[email protected]/node_modules/simple-git/src/lib/runners/git-executor-chain.ts:97:40)\n    at Generator.next (<anonymous>)\n    at fulfilled (/usr/local/renovate/node_modules/.pnpm/[email protected]/node_modules/simple-git/dist/cjs/index.js:52:24)\n    at processTicksAndRejections (node:internal/process/task_queues:95:5)"

My configuration is just setting gitPrivateKey to the gpg private key string.

@candrews candrews mentioned this pull request Aug 21, 2024
6 tasks
@viceice viceice added the regression Issue about a regression bug, or the PR caused it label Aug 22, 2024
@philippe-granet
Copy link

Hi @sisp , I try to use your feature, but I can't make it work, see #30999
Can you help us understand what's the problem?

@sisp
Copy link
Contributor Author

sisp commented Aug 25, 2024

It's a bug indeed. 🙇 See #30999 (reply in thread) and #31005.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
regression Issue about a regression bug, or the PR caused it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSH based commit and push signing
6 participants