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.

Sign up for GitHub

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

Jump to bottom

Update github.com/google/go-github to v51 #23946

Merged
merged 13 commits into from Apr 8, 2023
Merged

Update github.com/google/go-github to v51 #23946

merged 13 commits into from Apr 8, 2023

Conversation

harryzcy
Copy link
Contributor

@harryzcy harryzcy commented Apr 6, 2023

github.com/google/go-github has new major version releases frequently. It is required to update all import path, in additional to go.mod

@KN4CK3R
Copy link
Member

KN4CK3R commented Apr 6, 2023

CI fail is related.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 6, 2023
delvh
delvh approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Yup, just checked their changelog, you didn't miss any breaking change.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 6, 2023
@delvh
Copy link
Member

delvh commented Apr 6, 2023

CI fail is related.

Well, now it isn't anymore

@delvh delvh added this to the 1.20.0 milestone Apr 6, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 6, 2023
@delvh delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 6, 2023
@techknowlogick techknowlogick enabled auto-merge (squash) April 6, 2023 19:58
6543
6543 reviewed Apr 6, 2023
View reviewed changes
services/migrations/github.go Outdated
Updated: m.UpdatedAt,
Closed: m.ClosedAt,
Created: m.GetCreatedAt().Time,
Updated: &m.UpdatedAt.Time,
Copy link
Member

Choose a reason for hiding this comment

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

is there no GetTime func ?

Copy link
Member

Choose a reason for hiding this comment

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

nop

@jolheiser jolheiser removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 7, 2023
@jolheiser
Copy link
Member

It looks like the CI failures may still be related. 🙂

@6543 6543 disabled auto-merge April 7, 2023 19:06
@harryzcy
Copy link
Contributor Author

harryzcy commented Apr 8, 2023 •
edited

It looks like the CI failures may still be related. 🙂

Ok, I'm trying to reproduce it locally. But it seems the failing check is not running because GITHUB_READ_TOKEN is empty.

What is GITHUB_READ_TOKEN though? Is it any github token with read access?
Figured out.

github.com/google/go-github v50 changed all time types to github.Timestamp without providing a GetTime() method. So when accessing the underlying time.Time, the nil case should be checked first.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 8, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2023 •
edited

Codecov Report

Merging #23946 (1234f73) into main (f521e88) will decrease coverage by 0.09%.
The diff coverage is 30.85%.

@@            Coverage Diff             @@
##             main   #23946      +/-   ##
==========================================
- Coverage   47.14%   47.05%   -0.09%     
==========================================
  Files        1149     1163      +14     
  Lines      151446   153714    +2268     
==========================================
+ Hits        71397    72335     +938     
- Misses      71611    72868    +1257     
- Partials     8438     8511      +73
Impacted Files Coverage Δ
cmd/dump.go 0.66% <0.00%> (-0.01%) ⬇️
cmd/mailer.go 0.00% <0.00%> (ø)
cmd/manager.go 0.00% <0.00%> (ø)
cmd/manager_logging.go 0.00% <0.00%> (ø)
cmd/migrate_storage.go 5.76% <0.00%> (-0.12%) ⬇️
cmd/restore_repo.go 0.00% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.63% <0.00%> (-0.10%) ⬇️
models/actions/runner.go 1.44% <ø> (ø)
models/organization/org_user.go 80.51% <0.00%> (-12.02%) ⬇️
... and 65 more

... and 105 files with indirect coverage changes

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

@6543
Copy link
Member

6543 commented Apr 8, 2023

well I created a pull upstream google/go-github#2743 so we can use that later ...

@lunny lunny merged commit 1ee4530 into go-gitea:main Apr 8, 2023
2 checks passed
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 8, 2023
@harryzcy harryzcy deleted the update-github.com/google/go-github branch April 8, 2023 13:16
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 10, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
ac0dee9 
* upstream/main: (39 commits)
  Fix protected branch for API (go-gitea#24013)
  [skip ci] Updated translations via Crowdin
  Fix markdownlint (go-gitea#24024)
  Introduce lint-md and compliance-docs pipeline (go-gitea#24021)
  Fix https setup doc zh-cn (go-gitea#24015)
  Replace tribute with text-expander-element for textarea (go-gitea#23985)
  Improve GetBoards and getDefaultBoard (go-gitea#22981)
  Expand/Collapse all changed files (go-gitea#23639)
  Show errors for KaTeX and mermaid on the preview tab (go-gitea#24009)
  Show protected branch rule names again (go-gitea#23907)
  Reference the `zh-CN` version of `reverse-proxies` in `https-support` (go-gitea#24016)
  Fix lint problem in `https-support.zh-cn.md` (go-gitea#24014)
  docs: HTTPS configuration for zh-cn (go-gitea#23039)
  Re-add initial wiki page text when editing the page (go-gitea#23984)
  [skip ci] Updated translations via Crowdin
  fix: do not escape space between PyPI repository url and package name… (go-gitea#23981)
  Make bindata static build parse builtin templates correctly (go-gitea#24003)
  Group template helper functions, remove `Printf`, improve template error messages (go-gitea#23982)
  Adjust sticky pr header to cover background (go-gitea#23956)
  Update github.com/google/go-github to v51 (go-gitea#23946)
  ...
@6543 6543 mentioned this pull request Apr 10, 2023
Merged
silverwind added a commit that referenced this pull request May 31, 2023
@6543 @silverwind
Update github.com/google/go-github to v52 (#24004)
4c81dae 
based on google/go-github#2743

because of
#23946 (comment)

---------

Co-authored-by: silverwind <me@silverwind.io>
@wolfogre wolfogre mentioned this pull request Jun 14, 2023
Merged
wolfogre added a commit that referenced this pull request Jun 14, 2023
@wolfogre
Fix panic when migrating a repo from GitHub with issues (#25246)
6348823 
Fix #25245. Regression of #23946.
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Jun 14, 2023
@wolfogre @GiteaBot
Fix panic when migrating a repo from GitHub with issues (go-gitea#25246)
65a52e6 
Fix go-gitea#25245. Regression of go-gitea#23946.
@GiteaBot GiteaBot mentioned this pull request Jun 14, 2023
Merged
silverwind pushed a commit that referenced this pull request Jun 14, 2023
@GiteaBot @wolfogre
Fix panic when migrating a repo from GitHub with issues (#25246) (#25247
8fa9d9d 
)

Backport #25246 by @wolfogre

Fix #25245. Regression of #23946.

Co-authored-by: Jason Song <i@wolfogre.com>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KN4CK3R KN4CK3R KN4CK3R approved these changes

@6543 6543 6543 approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@harryzcy @KN4CK3R @delvh @jolheiser @codecov-commenter @6543 @lunny @GiteaBot @silverwind