Skip to content

Remove openpgp and change CreateCommit signature#2935

Merged
gmlewis merged 2 commits intogoogle:masterfrom
WillAbides:signatory
Sep 21, 2023
Merged

Remove openpgp and change CreateCommit signature#2935
gmlewis merged 2 commits intogoogle:masterfrom
WillAbides:signatory

Conversation

@WillAbides
Copy link
Copy Markdown
Contributor

closes #2932

In #2932 we discussed removing the dependency on openpgp and replacing Commit.SigningKey with an interface.

While I was working on it, I realized that this should be an option to CreateCommit instead of a Commit field. I was going to just ignore this, but adding an interface field to Commit caused issues with Stringify and the generated string tests. Instead of unwinding all that, I'm doing the easy thing first and adding a CreateCommitOptions struct. I can change approaches if necessary.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 19, 2023

Codecov Report

Merging #2935 (fd6be71) into master (b45ef89) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2935   +/-   ##
=======================================
  Coverage   98.17%   98.17%           
=======================================
  Files         143      143           
  Lines       12597    12602    +5     
=======================================
+ Hits        12367    12372    +5     
  Misses        156      156           
  Partials       74       74           
Files Changed Coverage Δ
github/git_commits.go 100.00% <100.00%> (ø)

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Awesome, @WillAbides !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

Comment on lines +156 to +158
opts.Signer = github.MessageSignerFunc(func(w io.Writer, r io.Reader) error {
return openpgp.ArmoredDetachSign(w, key, r, nil)
})
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

TIL that you can wrap a single func in a type cast that can then be used as a receiver in order to satisfy an interface...
so cool, @WillAbides - thank you!

@gmlewis gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Sep 19, 2023
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Sep 21, 2023
@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Sep 21, 2023

Thank you, @valbeat !
Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transitive dependency (cloudflare/circl) making cross compiling difficult

3 participants