-
Notifications
You must be signed in to change notification settings - Fork 2k
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 #1198
Conversation
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.
Thank you, @anandkumarpatel.
I've requested some changes, please, when you have time.
github/git_commits.go
Outdated
message := "" | ||
|
||
if commit.Tree != nil { | ||
message = fmt.Sprintf("tree %s\n", *commit.Tree) |
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.
Please use the getters... commit.GetTree()
.
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.
There are no getters defined for createCommit
object
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.
Yes, whups, my bad. Thanks, @anandkumarpatel.
@gmlewis fixed most of the comments you left. The only issue was with using getters on the I also updated all the short names in the test file and defined them more verbosely. |
Just FYI... the other names were fine. As you'll see in a lot of other tests, we use But it was just a bit challenging at a quick glance to figure out in the tests that:
was the commit message. Thanks. |
@gmlewis ready for round 2 👊 review.
|
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.
Thank you, @anandkumarpatel!
Just a couple minor tweaks, please, and then I think we are good to go.
@gmlewis Just pushed, lmk if there is something else. |
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.
Thank you for bearing with me, @anandkumarpatel!
I think this is going to turn out nicely. Just a couple more minor tweaks, please.
@gmlewis Fixed the issues.
|
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.
Thank you, @anandkumarpatel!
LGTM.
Awaiting second LGTM before merging.
friendly ping @gauntface |
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.
Mostly LGTM, only question I had is regarding the expected behavior if you set commit.SigningKey
AND commit.Verification
.
At the moment the signature will be commit.Verification
, is that expected? Would it be better if we returned an error if both are set?
@gauntface I would assume if Are my assumptions invalid? |
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'll LGTM as is. Thinking on it more, I'm just projecting my general like of enforced input -> behavior regarding the inputs. It may never be an issue.
Good discussion. Thanks, @gauntface and @anandkumarpatel ! |
Thank you, @anandkumarpatel! |
Reopening of #1142
This PR creates a new param for the
Commit
struct calledSignKey
.SignKey
is anopenpgp.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 definedSignKey
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
So I wanted to make it less complicated 😄