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

Move to Protonmail PGP #2666

Merged
merged 7 commits into from Mar 8, 2023
Merged

Move to Protonmail PGP #2666

merged 7 commits into from Mar 8, 2023

Conversation

komod0
Copy link
Contributor

@komod0 komod0 commented Feb 18, 2023

This changes the openpgp library so that it uses the Protonmail fork which seems to be the preferred one and it's the one go-git has adopted.
Closes #2317

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Feb 19, 2023
@codecov
Copy link

codecov bot commented Feb 19, 2023

Codecov Report

Merging #2666 (be89d17) into master (e0bb535) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2666   +/-   ##
=======================================
  Coverage   98.05%   98.05%           
=======================================
  Files         131      131           
  Lines       11459    11459           
=======================================
  Hits        11236    11236           
  Misses        152      152           
  Partials       71       71           
Impacted Files Coverage Δ
github/git_commits.go 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 19, 2023

I'm not sure why the tests are failing... all I could find is this: golangci/golangci-lint#825
but that doesn't appear to match what is going on here.
Running golangci-lint run manually (locally) seems fine. Very confusing.

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 21, 2023

When I check out your branch locally, I'm seeing this:

glenn@glenn-OMEN-875 ~/go/src/github.com/google/go-github (komod0-chore/movepgp) $ ./test-all.sh 
+ export GOROOT=/usr/local/go
+ GOROOT=/usr/local/go
+ diff -u /dev/fd/63 /dev/fd/62
++ echo -n
++ gofmt -d -s .
--- /dev/fd/63	2023-02-21 08:03:34.134780783 -0500
+++ /dev/fd/62	2023-02-21 08:03:34.134780783 -0500
@@ -0,0 +1,26 @@
+diff github/repos_commits_test.go.orig github/repos_commits_test.go
+--- github/repos_commits_test.go.orig
++++ github/repos_commits_test.go
+@@ -14,8 +14,8 @@
+ 	"testing"
+ 	"time"
+ 
+-	"github.com/google/go-cmp/cmp"
+ 	"github.com/ProtonMail/go-crypto/openpgp"
++	"github.com/google/go-cmp/cmp"
+ )
+ 
+ func TestRepositoriesService_ListCommits(t *testing.T) {
+diff github/repos_contents_test.go.orig github/repos_contents_test.go
+--- github/repos_contents_test.go.orig
++++ github/repos_contents_test.go
+@@ -14,8 +14,8 @@
+ 	"net/url"
+ 	"testing"
+ 
+-	"github.com/google/go-cmp/cmp"
+ 	"github.com/ProtonMail/go-crypto/openpgp"
++	"github.com/google/go-cmp/cmp"
+ )
+ 
+ func TestRepositoryContent_GetContent(t *testing.T) {
glenn@glenn-OMEN-875 ~/go/src/github.com/google/go-github (komod0-chore/movepgp) $ go version
go version go1.19.5 linux/amd64

What do you see?

@komod0
Copy link
Contributor Author

komod0 commented Feb 21, 2023

Where can i find the test_all.sh script to try that?

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 22, 2023

Where can i find the test_all.sh script to try that?

It's nothing special... just something I threw together to help me out with this repo:

$ cat test-all.sh 
#!/bin/bash -ex

# https://github.com/golangci/golangci-lint/issues/3107
export GOROOT=/usr/local/go

diff -u <(echo -n) <(gofmt -d -s .)
go mod tidy
go generate ./...
go test ./...
go vet ./...
golangci-lint run

cd scrape
go mod tidy
go generate ./...
go test ./...
go vet ./...
golangci-lint run

cd ../update-urls
go mod tidy
go generate ./...
go test ./...
go vet ./...
golangci-lint run

@komod0
Copy link
Contributor Author

komod0 commented Feb 22, 2023

I think it's similar except the version

❯ ./test_all.sh
+ export GOROOT=/usr/local/go
+ GOROOT=/usr/local/go
+ diff -u /dev/fd/63 /dev/fd/62
++ echo -n
++ gofmt -d -s .
--- /dev/fd/63  2023-02-22 01:15:08
+++ /dev/fd/62  2023-02-22 01:15:10
@@ -0,0 +1,26 @@
+diff -u github/repos_commits_test.go.orig github/repos_commits_test.go
+--- github/repos_commits_test.go.orig  2023-02-22 01:15:10
++++ github/repos_commits_test.go       2023-02-22 01:15:10
+@@ -14,8 +14,8 @@
+       "testing"
+       "time"
+ 
+-      "github.com/google/go-cmp/cmp"
+       "github.com/ProtonMail/go-crypto/openpgp"
++      "github.com/google/go-cmp/cmp"
+ )
+ 
+ func TestRepositoriesService_ListCommits(t *testing.T) {
+diff -u github/repos_contents_test.go.orig github/repos_contents_test.go
+--- github/repos_contents_test.go.orig 2023-02-22 01:15:10
++++ github/repos_contents_test.go      2023-02-22 01:15:10
+@@ -14,8 +14,8 @@
+       "net/url"
+       "testing"
+ 
+-      "github.com/google/go-cmp/cmp"
+       "github.com/ProtonMail/go-crypto/openpgp"
++      "github.com/google/go-cmp/cmp"
+ )
+ 
+ func TestRepositoryContent_GetContent(t *testing.T) {
❯ go version
go version go1.17.12 darwin/amd64

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 22, 2023

OK, then please try running gofmt on those files that you modified to see if that fixes the payment then push the changes to this PR.

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 6, 2023

@komod0 - please try merging in the latest changes to this repo. I'm hoping that #2694 will fix the errors being reported.

@komod0
Copy link
Contributor Author

komod0 commented Mar 6, 2023

@gmlewis Done

Copy link
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.

Thank you, @komod0 !
LGTM.

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

This was referenced Mar 6, 2023
Copy link
Contributor

@valbeat valbeat left a comment

Choose a reason for hiding this comment

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

LGTM!

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 8, 2023

Thank you, @valbeat !
@komod0 - would you like to resolve the go.mod/go.sum conflicts yourself by typing:

git checkout master
git pull
git checkout chore/movepgp
git merge master
go mod tidy -compat=1.17
git push origin chore/movepgp

or would you like me to do it?

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Mar 8, 2023
@komod0
Copy link
Contributor Author

komod0 commented Mar 8, 2023

@gmlewis Hey!, i just saw your comment, i fixed the conflicts manually and run go mod tidy after that, i think it should be the same

@gmlewis gmlewis merged commit 8e34527 into google:master Mar 8, 2023
exageraldo pushed a commit to exageraldo/go-github that referenced this pull request Mar 13, 2023
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.

golang.org/x/crypto/openpgp is deprecated
3 participants