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 SignKey gpg entity param to allow easier pgp signing of commits #1142

Closed

Conversation

anandkumarpatel
Copy link
Contributor

@anandkumarpatel anandkumarpatel commented Apr 2, 2019

This PR creates a new param for the Commit struct called SignKey.
SignKey is an openpgp.Entity type which is the standard way of reading and validating pgp keys.

When specified the key will be used to sign the commit. This makes it so the user does not have to sign the key externally and duplicate some of the logic the CreateCommit function does.

To maintain backward compatibility, if the Verification.Signature is defined SignKey will have no effect.

This provides similar functionality that go-git has: src-d/go-git@7b6c126

Note from the git docs: https://github.com/octokit/routes/blob/152c0b5d156844e17a1805d0509c9a0b7cfc848f/routes/api.github.com/git.json#L282

To pass a `signature` parameter, you need to first manually create a valid PGP signature, which can be complicated. You may find it easier to [use the command line]

So I wanted to make it less complicated 😄

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Apr 2, 2019
@codecov
Copy link

codecov bot commented Apr 2, 2019

Codecov Report

Merging #1142 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1142      +/-   ##
==========================================
+ Coverage   70.18%   70.32%   +0.14%     
==========================================
  Files          84       84              
  Lines        5792     5820      +28     
==========================================
+ Hits         4065     4093      +28     
  Misses        947      947              
  Partials      780      780
Impacted Files Coverage Δ
github/git_commits.go 90.9% <100%> (+6.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14c49e1...307fef9. Read the comment docs.

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 2, 2019

Thank you, @anandkumarpatel.

In the past, when "helper" functions have been suggested for this repo, we have typically requested that they be put in their own separate repo for a few reasons: it keeps the API surface area of this repo smaller, reduces the maintenance burden on this repo, and keeps its list of external dependencies smaller.

However, I can see how this can be useful, and I'm game to include it in this repo.

I'd like @willnorris and @gauntface to weigh in on their thoughts for including something like this first.

Copy link
Collaborator

@willnorris willnorris left a comment

Choose a reason for hiding this comment

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

I don't love the idea of adding this into the Commit type like this, but given how CreateCommit works (internally creating a createCommit struct which carries the signature, rather than the signature being on the passed in Commit struct directly), there's not really an option I can think of that's all that much better.

Looks fine to me in principle. I haven't thoroughly reviewed or tested the actual signature generation logic, though.

github/git_commits.go Show resolved Hide resolved
@gmlewis
Copy link
Collaborator

gmlewis commented Jun 14, 2019

Closing abandoned PR.

@gmlewis gmlewis closed this Jun 14, 2019
@anandkumarpatel
Copy link
Contributor Author

@gmlewis Sorry, forgot about this one.
I rebased and updated my branch based on @willnorris feedback. Is it possible to reopen? or should I make another PR?

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 14, 2019

Oh, sure... no problem! I was just trying to do some Spring Cleaning in June. 😄

@gmlewis
Copy link
Collaborator

gmlewis commented Jun 14, 2019

Screenshot from 2019-06-14 09-40-42

Whups, it's not allowing me to reopen because of a force push. Can you please open another PR with these changes and we can run with that one?

@anandkumarpatel
Copy link
Contributor Author

anandkumarpatel commented Jun 14, 2019

No problem! Continued here: #1198

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants