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

add git commit signing #127

Closed
wants to merge 6 commits into from
Closed

add git commit signing #127

wants to merge 6 commits into from

Conversation

jackleslie
Copy link
Contributor

@jackleslie jackleslie commented Dec 18, 2021

Context

Fixes #126

Changes

  • Add a githubUserName input option
    • Default to "github-actions[bot]"
  • Add a githubUserEmail input option
    • Default to "github-actions[bot]@users.noreply.github.com"
  • Add a GPG_PRIVATE_KEY env option
    • This determines if keys should be signed or not
  • Pass in githubUserName and githubUserEmail to setupUser
    • Can be done outwith signed commits, but these are needed to verify the signed commits if its enabled
  • Add a setupCommitSigning if GPG_PRIVATE_KEY is present
    • Need to install gpg
    • use STDIN for gpg --import (utf-8 encoding with Buffer.from by default)
    • Need to do a /bin/bash command to get and grep the key id - exec doesn't support piping and multiple commands on its own
    • Set git config for user.signingkey and commit.gpgsign

Considerations

  • Input option naming (githubUserName vs github_user_name)
  • Should the input options just be more env variables instead?]
  • How do you feel about installing gpg here?

@changeset-bot
Copy link

changeset-bot bot commented Dec 18, 2021

🦋 Changeset detected

Latest commit: 744ef55

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@changesets/action Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

src/gitUtils.ts Outdated Show resolved Hide resolved
src/gitUtils.ts Outdated
"gnupg",
]);
await exec("gpg", ["--import"], { input: Buffer.from(gpgPrivateKey) });
const { stdout: keyId } = await execWithOutput(`/bin/bash -c "gpg --list-secret-keys --with-colons | grep '^sec:' | cut -d ':' -f 5"`);
Copy link
Member

Choose a reason for hiding this comment

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

couldnt this be decoded from the importing's command output? something like this should display the information about what is being imported

gpg --with-colons --import-options show-only --import

the problem with listing all keys is that potentially the returned list could potentially contain many entries and we could grab some unrelated keyId here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah true, although the current implementation seems to be sufficient for the crowdin action, especially if consumers are just passing the one private key.

Happy to explore your solution though if this is a blocker.

Copy link
Member

Choose a reason for hiding this comment

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

It's not a blocker per se but if this solution could be used then I would prefer using it - cause it seems to be more explicit about the whole flow. It's less dependent on other factors and the preexisting environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense, I won't pick this up for a few days now but I've addressed all the other comments, if it's sufficient as is then feel free to merge but if not I'll come back to this

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@emmatown
Copy link
Member

Could we just have an option to disable setting the git email and name and then you can setup the signing before the Changesets action runs? It seems strange to have this built-in to the Changesets action.

@jackleslie
Copy link
Contributor Author

@mitchellhamilton yeah that's valid, I'm happy to open up a new PR with that approach if you prefer.

The current implementation is based on the Crowdin action, so it's up to you whether you'd like to align with that or if you'd rather have the action assume the git user is set up.

@jackleslie
Copy link
Contributor Author

@mitchellhamilton here's another PR with the solution you suggested

Let me know which approach you both prefer

@jackleslie
Copy link
Contributor Author

jackleslie commented Jan 6, 2022

Closing in favour of #131

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.

Add option to enable commit signing
3 participants