-
Notifications
You must be signed in to change notification settings - Fork 5
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
ci: signed commits #95
base: master
Are you sure you want to change the base?
Conversation
@@ -37,11 +38,21 @@ jobs: | |||
run: yarn install --immutable | |||
- name: Prepare | |||
run: yarn prepare | |||
- name: Import GPG key | |||
id: import-gpg | |||
uses: crazy-max/ghaction-import-gpg@1a317071222a9bfb1839df1b58b1f0dcd893b589 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@633kh4ck could u have a look at this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a review and I can't see any malicious attempts at trying to steal the keys. Note that I didn't review the openpgp
package which has access to the private key here: https://github.com/crazy-max/ghaction-import-gpg/blob/60f6f3e9a98263cc2c51ebe1f9babe82ded3f0ba/src/openpgp.ts#L18-L20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to merge for now? the consequence of that GPG key leaking wouldn't be dramatic imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @633kh4ck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need a thirdparty action for this / why can't this be a command?
ping @633kh4ck |
we can stop using the 3rd party action and go for a vanilla approach instead such as - name: Configure GPG Key
run: |
echo -n "${{ secrets. GPG_PRIVATE_KEY }}" | base64 --decode | gpg --import
git config --global user.signingkey ABCD1234 |
It looks safer and cleaner at first glance (or we need to audit third-party code, including |
Houston, this is Conflict Bot. We have a conflict. I repeat, we have a conflict. @sparten11740 please rebase. Acknowledge. |
Single-file updates: https://docs.github.com/en/rest/repos/contents#create-or-update-file-contents (you also want Fine-grained API which can build a commit with changes across multiple files: https://docs.github.com/en/rest/git/commits#create-a-commit |
@marcoskichel we currently use git here but looks like we can use the GH API to get signed commits. Could you have a look when you have time? |
Changing the files through the API has two disadvantages: it's subject to API rate limits (not a big deal if we only do it in this repo) and which is more annoying: we need to be explicit as to what files need updating. In this case all package.json's and changelogs of the released packages and potentially the lockfile. I find the git cli with signed commits more appealing tbh |
@sparten11740 does this PR need to be updated to use https://github.com/ExodusMovement/git-signing-action ? |
Yes, but we first have to make the action repo public. Any objections? |
just asked on Slack in #ossing-requests |
Our branch protection rules require us to use signed commits. This adds changes the committer to the bot that creates the PR and enables GPG signing
release
env and addGPG_PRIVATE_KEY
andGPG_PASSPHRASE
as secretsTestplan
Tested in https://github.com/ExodusMovement/lerna-version-selectively/pull/504