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

Migration failure during reaction migration from gitea #13344

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Oct 28, 2020

There are two problems in #13262 which is a situation whereby a migration fails due to reactions migration failing.

  1. A failure during migrating reactions should not cause failure of migration - yes they're nice but really?!
  2. The underlying reaction failure is due to the API checking the wrong permission for pull requests.
  3. And the same error was present on the API for comment reactions...

Fix #13262

Signed-off-by: Andrew Thornton [email protected]

A failure during migrating reactions should not cause failure of
migration.

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath
Copy link
Contributor Author

zeripath commented Oct 28, 2020

I've made this backport to v1.12 - only for the second & third commit in this PR - the reactions API should check the correct permission.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 28, 2020
@zeripath zeripath changed the title Migrating reactions is just not that important Migration failure during reaction migration from gitea Oct 28, 2020
@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 Oct 29, 2020
@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 Oct 29, 2020
@6543
Copy link
Member

6543 commented Oct 29, 2020

🚀

@6543 6543 added the topic/repo-migration Migrate repos from other platforms to Gitea, or from Gitea to them label Oct 29, 2020
@techknowlogick
Copy link
Member

🚀

@codecov-io
Copy link

Codecov Report

Merging #13344 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13344      +/-   ##
==========================================
- Coverage   42.11%   42.11%   -0.01%     
==========================================
  Files         689      689              
  Lines       75845    75851       +6     
==========================================
  Hits        31943    31943              
- Misses      38668    38674       +6     
  Partials     5234     5234              
Impacted Files Coverage Δ
modules/migrations/gitea_downloader.go 0.92% <0.00%> (-0.02%) ⬇️
modules/migrations/migrate.go 21.55% <0.00%> (ø)
routers/api/v1/repo/issue_reaction.go 47.44% <0.00%> (ø)
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/queue/workerpool.go 56.73% <0.00%> (-2.05%) ⬇️
services/pull/pull.go 40.78% <0.00%> (-0.50%) ⬇️
modules/git/repo.go 46.70% <0.00%> (+0.50%) ⬆️
services/pull/check.go 52.55% <0.00%> (+0.72%) ⬆️
modules/git/utils.go 77.04% <0.00%> (+3.27%) ⬆️
modules/charset/charset.go 73.03% <0.00%> (+4.49%) ⬆️
... and 1 more

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 beb6bf4...6cd5446. Read the comment docs.

@techknowlogick techknowlogick merged commit 4b33afc into go-gitea:master Oct 29, 2020
techknowlogick added a commit to techknowlogick/gitea that referenced this pull request Oct 29, 2020
* Migrating reactions is just not that important

A failure during migrating reactions should not cause failure of
migration.

Signed-off-by: Andrew Thornton <[email protected]>

* When checking issue reactions check the correct permission

Signed-off-by: Andrew Thornton <[email protected]>

Co-authored-by: techknowlogick <[email protected]>
@techknowlogick techknowlogick added the backport/done All backports for this PR have been created label Oct 29, 2020
techknowlogick added a commit to techknowlogick/gitea that referenced this pull request Oct 29, 2020
* Migrating reactions is just not that important

A failure during migrating reactions should not cause failure of
migration.

Signed-off-by: Andrew Thornton <[email protected]>

* When checking issue reactions check the correct permission

Signed-off-by: Andrew Thornton <[email protected]>

Co-authored-by: techknowlogick <[email protected]>
@zeripath zeripath deleted the migrating-reactions-is-just-not-important branch October 29, 2020 02:45
@zeripath
Copy link
Contributor Author

Damn this is missing the last commit!

There's another place in the same file that has the wrong check

@zeripath
Copy link
Contributor Author

https://github.com/zeripath/gitea/blob/6cd544638a36f010b1ae32f768ecf37b74aa8f7f/routers/api/v1/repo/issue_reaction.go#L60

Is also the wrong permission check. It appears that the last commit in the branch didn't get pushed up - not sure why!

techknowlogick added a commit that referenced this pull request Oct 29, 2020
* Migrating reactions is just not that important

A failure during migrating reactions should not cause failure of
migration.

Signed-off-by: Andrew Thornton <[email protected]>

* When checking issue reactions check the correct permission

Signed-off-by: Andrew Thornton <[email protected]>

Co-authored-by: techknowlogick <[email protected]>

Co-authored-by: zeripath <[email protected]>
zeripath added a commit to techknowlogick/gitea that referenced this pull request Oct 29, 2020
Unfortunately my final push to go-gitea#13344 didn't register - or I failed to push it properly. GetIssueCommentReactions in routers/api/v1/repo/issue_reaction.go also makes the same mistake.
@6543
Copy link
Member

6543 commented Oct 29, 2020

v1.12 backport is #13346 @techknowlogick why did you remove the label?

techknowlogick added a commit that referenced this pull request Oct 29, 2020
* Migration failure during reaction migration from gitea (#13344)

* Migrating reactions is just not that important

A failure during migrating reactions should not cause failure of
migration.

Signed-off-by: Andrew Thornton <[email protected]>

* When checking issue reactions check the correct permission

Signed-off-by: Andrew Thornton <[email protected]>

Co-authored-by: techknowlogick <[email protected]>

* Delete gitea_downloader.go

* Backport #13349

Unfortunately my final push to #13344 didn't register - or I failed to push it properly. GetIssueCommentReactions in routers/api/v1/repo/issue_reaction.go also makes the same mistake.

Co-authored-by: zeripath <[email protected]>
@6543 6543 added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Nov 10, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/repo-migration Migrate repos from other platforms to Gitea, or from Gitea to them type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration with pull requests from other gitea instance fails
6 participants