Skip to content

Fix review request webhook bug (#35339) #35596

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
Open
wants to merge 5 commits into
base: release/v1.25
Choose a base branch
from
Open

Fix review request webhook bug (#35339) #35596

wants to merge 5 commits into from
+111 −30

Conversation

@lunny
Copy link
Member

@lunny lunny commented Oct 6, 2025

Frontport from #35339

@lunny lunny added this to the 1.25.0 milestone Oct 6, 2025
@lunny lunny added the type/bug label Oct 6, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 6, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Oct 6, 2025
@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 10, 2025
@wxiaoguang
Copy link
Contributor

The main fix isn't merged yet, the backports are there? Move request review functions to pull service package #35337

Doesn't it violate the development workflow?

@lunny
Copy link
Member Author

lunny commented Oct 16, 2025

The main fix isn't merged yet, the backports are there? Move request review functions to pull service package #35337

Doesn't it violate the development workflow?

#35337 includes many refactors but this PR only fix the bugs. So it's a partial backport.

@wxiaoguang
Copy link
Contributor

The main fix isn't merged yet, the backports are there? Move request review functions to pull service package #35337
Doesn't it violate the development workflow?

#35337 includes many refactors but this PR only fix the bugs. So it's a partial backport.

What ever you explain, I don't think it is the expected result that "a bug has been fixed in 1.24 for two months, but 1.25 just got its unmerged PR a few days before, and no idea when 1.26 can get its fix".

@lunny
Copy link
Member Author

lunny commented Oct 17, 2025

The main fix isn't merged yet, the backports are there? Move request review functions to pull service package #35337
Doesn't it violate the development workflow?

#35337 includes many refactors but this PR only fix the bugs. So it's a partial backport.

What ever you explain, I don't think it is the expected result that "a bug has been fixed in 1.24 for two months, but 1.25 just got its unmerged PR a few days before, and no idea when 1.26 can get its fix".

The wrong behavior is merging #35339 before #35337. At that time, the target of #35337 is for 1.25.
At the moment, I think we have to frontport #35339 to v1.25 and review #35337 ASAP.

@lunny
Copy link
Member Author

lunny commented Oct 22, 2025

#35337

Sent #35723 to the main branch

yp05327
yp05327 reviewed Oct 24, 2025
View reviewed changes
services/pull/comment.go
}

// CreatePushPullComment create push code to pull base comment
// CreatePushPullComment create push code to pull base comment, if no diff in this pr, no comment created
Copy link
Contributor

@yp05327 yp05327 Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

PLZ no new changes in backport, or also adding it into the main branch (later)

@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 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yp05327 yp05327 yp05327 approved these changes

+1 more reviewer

@Zettat123 Zettat123 Zettat123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code type/bug

Projects

None yet

Milestone

1.25.0

Development

Successfully merging this pull request may close these issues.

5 participants

@lunny @wxiaoguang @Zettat123 @yp05327 @GiteaBot