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

Retarget depending pulls when the parent branch is deleted #28686

Merged
merged 17 commits into from
Jan 17, 2024

Conversation

kvaster
Copy link
Contributor

@kvaster kvaster commented Jan 3, 2024

Sometimes you need to work on a feature which depends on another (unmerged) feature.
In this case, you may create a PR based on that feature instead of the main branch.
Currently, such PRs will be closed without the possibility to reopen in case the parent feature is merged and its branch is deleted.
Automatic target branch change make life a lot easier in such cases.
Github and Bitbucket behave in such way.

Example:
$PR_1$: main <- feature1
$PR_2$: feature1 <- feature2

Currently, merging $PR_1$ and deleting its branch leads to $PR_2$ being closed without the possibility to reopen.
This is both annoying and loses the review history when you open a new PR.

With this change, $PR_2$ will change its target branch to main ($PR_2$: main <- feature2) after $PR_1$ has been merged and its branch has been deleted.

This behavior is enabled by default but can be disabled.
For security reasons, this target branch change will not be executed when merging PRs targeting another repo.

Fixes #27062
Fixes #18408

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 3, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 3, 2024
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/docs labels Jan 3, 2024
@denyskon denyskon self-requested a review January 3, 2024 19:40
@kvaster
Copy link
Contributor Author

kvaster commented Jan 3, 2024

@denyskon , my advice to review this PR.

And if you read previous thread:

  • I was against feature flag - it can be removed, cause such behaviour seems to be normal for any case
  • I'm not able to write proper unit tests right now as I was requested
  • Child PR's are not retargeted in case Base PR targets another repo

Copy link
Member

@denyskon denyskon left a comment

Choose a reason for hiding this comment

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

I am not against making this feature optional, but definitely enabled by default. I think it is good that retarget only occurs inside the same repo, because otherwise PRs may end up somewhere they weren't supposed to.

custom/conf/app.example.ini Outdated Show resolved Hide resolved
docs/content/administration/config-cheat-sheet.en-us.md Outdated Show resolved Hide resolved
modules/setting/repository.go Outdated Show resolved Hide resolved
@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 Jan 14, 2024
@denyskon
Copy link
Member

@kvaster If we need a test, you can add me as collaborator to your fork, then I'd try to write one.

@denyskon denyskon added type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/pr Issues related to pull requests labels Jan 14, 2024
@denyskon denyskon added this to the 1.22.0 milestone Jan 14, 2024
@denyskon
Copy link
Member

@kvaster Could you mention closing #27062 in PR description? It has a better description of the feature, IMO

@denyskon denyskon requested review from a1012112796 and delvh January 14, 2024 12:28
@kvaster kvaster changed the title Retarget Pulls Based on Pulls on Merge Retarget Pulls Based on Pulls on Merge, fixes #27062, #18408 Jan 15, 2024
@kvaster
Copy link
Contributor Author

kvaster commented Jan 15, 2024

I am not against making this feature optional, but definitely enabled by default. I think it is good that retarget only occurs inside the same repo, because otherwise PRs may end up somewhere they weren't supposed to.

I think that making this as a feature is non usefull at all. I will remove with please feature gate if gitea authors/maintainers are ok with this. So, waiting for agreement on this.

@denyskon, adding you as a collaborator.

@denyskon
Copy link
Member

It's normal for Gitea to be able to disable everything you want, so I don't see any reason not to allow it here too 😆 It is already implemented, so let's keep it, but enable by default.

I'll try to take care of the test later today

@kvaster
Copy link
Contributor Author

kvaster commented Jan 15, 2024

Great. Will update PR in a few minutes.

services/pull/pull.go Outdated Show resolved Hide resolved
services/pull/pull.go Show resolved Hide resolved
@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 Jan 15, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 16, 2024
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.

I've finished testing, everything behaves exactly as expected 🎉

docs/content/administration/config-cheat-sheet.en-us.md Outdated Show resolved Hide resolved
custom/conf/app.example.ini Outdated Show resolved Hide resolved
@delvh delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 16, 2024
@delvh delvh merged commit 49eb168 into go-gitea:main Jan 17, 2024
25 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 17, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 17, 2024
* giteaofficial/main:
  Retarget depending pulls when the parent branch is deleted (go-gitea#28686)
  Bump `@github/relative-time-element` to 4.3.1 (go-gitea#28819)
  Fix reverting a merge commit failing (go-gitea#28794)
  Render code block in activity tab (go-gitea#28816)
  Remove trust model selection from repository creation on web page because it can be changed in settings later (go-gitea#28814)
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
…28686)

Sometimes you need to work on a feature which depends on another (unmerged) feature.
In this case, you may create a PR based on that feature instead of the main branch.
Currently, such PRs will be closed without the possibility to reopen in case the parent feature is merged and its branch is deleted.
Automatic target branch change make life a lot easier in such cases.
Github and Bitbucket behave in such way.

Example:
$PR_1$: main <- feature1
$PR_2$: feature1 <- feature2

Currently, merging $PR_1$ and deleting its branch leads to $PR_2$ being closed without the possibility to reopen.
This is both annoying and loses the review history when you open a new PR.

With this change, $PR_2$ will change its target branch to main ($PR_2$: main <- feature2) after $PR_1$ has been merged and its branch has been deleted.

This behavior is enabled by default but can be disabled.
For security reasons, this target branch change will not be executed when merging PRs targeting another repo. 

Fixes go-gitea#27062
Fixes go-gitea#18408

---------

Co-authored-by: Denys Konovalov <kontakt@denyskon.de>
Co-authored-by: delvh <dev.lh@web.de>
henrygoodman pushed a commit to henrygoodman/gitea that referenced this pull request Jan 31, 2024
…28686)

Sometimes you need to work on a feature which depends on another (unmerged) feature.
In this case, you may create a PR based on that feature instead of the main branch.
Currently, such PRs will be closed without the possibility to reopen in case the parent feature is merged and its branch is deleted.
Automatic target branch change make life a lot easier in such cases.
Github and Bitbucket behave in such way.

Example:
$PR_1$: main <- feature1
$PR_2$: feature1 <- feature2

Currently, merging $PR_1$ and deleting its branch leads to $PR_2$ being closed without the possibility to reopen.
This is both annoying and loses the review history when you open a new PR.

With this change, $PR_2$ will change its target branch to main ($PR_2$: main <- feature2) after $PR_1$ has been merged and its branch has been deleted.

This behavior is enabled by default but can be disabled.
For security reasons, this target branch change will not be executed when merging PRs targeting another repo. 

Fixes go-gitea#27062
Fixes go-gitea#18408

---------

Co-authored-by: Denys Konovalov <kontakt@denyskon.de>
Co-authored-by: delvh <dev.lh@web.de>
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
…28686)

Sometimes you need to work on a feature which depends on another (unmerged) feature.
In this case, you may create a PR based on that feature instead of the main branch.
Currently, such PRs will be closed without the possibility to reopen in case the parent feature is merged and its branch is deleted.
Automatic target branch change make life a lot easier in such cases.
Github and Bitbucket behave in such way.

Example:
$PR_1$: main <- feature1
$PR_2$: feature1 <- feature2

Currently, merging $PR_1$ and deleting its branch leads to $PR_2$ being closed without the possibility to reopen.
This is both annoying and loses the review history when you open a new PR.

With this change, $PR_2$ will change its target branch to main ($PR_2$: main <- feature2) after $PR_1$ has been merged and its branch has been deleted.

This behavior is enabled by default but can be disabled.
For security reasons, this target branch change will not be executed when merging PRs targeting another repo. 

Fixes go-gitea#27062
Fixes go-gitea#18408

---------

Co-authored-by: Denys Konovalov <kontakt@denyskon.de>
Co-authored-by: delvh <dev.lh@web.de>
@MGChecker
Copy link

Maybe one should take a look how retargeting like this affects CI integrations like Drone, in particular. I remember having observed issues regarding that in the past.

@delvh
Copy link
Member

delvh commented Feb 25, 2024

What do you mean?
What sort of issues?

@6543 6543 added the type/changelog Adds the changelog for a new Gitea version label Feb 25, 2024
@6543
Copy link
Member

6543 commented Feb 25, 2024

If there is a problem with integration with other software, just open a new issue and we see how we can address it on our side.

For your specific note, tbh. that sounds more like a bug in drone-ci ... but if it realy ocures, let us know :)

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Feb 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/docs size/L Denotes a PR that changes 100-499 lines, ignoring generated files. topic/pr Issues related to pull requests type/changelog Adds the changelog for a new Gitea version type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing the destination branch of a pull request after deletion Allow cascade PRs
7 participants