feat: when using replace strategy also update PR #368
Conversation
There was a problem hiding this comment.
Thanks for the PR!
The code looks really good! I don't really have any code comments other than that we might be able to share some code between the Create and Update functions. But it is probably also as much of premature optimization, so I did not actually put in any comments on it.
The thing that needs to be discussed is what should happen to fields such as reviewers/labels/etc.
It seems that the behavior differs between SCMs (and for Giteas case, per field type). In some cases, the fields are updated to the list of the values provided to multi-gitter, while in other cases, the new values are added to any existing ones.
- Gitlab seems to replace the labels
multi-gitter/internal/scm/gitlab/gitlab.go
Line 329 in 922787a
- While GitHub adds the new ones
multi-gitter/internal/scm/github/github.go
Line 398 in 922787a
I think the replacement approach seems more logical. But I'm happy to discuss which one should be used. Either way, it should be the same no matter of SCM/field type.
| Reviewers: getReviewers(r.Reviewers, r.MaxReviewers), | ||
| TeamReviewers: getReviewers(r.TeamReviewers, r.MaxTeamReviewers), |
There was a problem hiding this comment.
Thought/Discussion: If max-reviewers is less than the reviewers provided, the reviewers used will be randomized. Do we want this to happen in the case of an update or not? 🤔 I have no real opinion here, but a conscious decision has to be made.
lindell
force-pushed
the
master
branch
2 times, most recently
from
de07efa to
2e9b182
Compare
|
This is a pretty useful PR that would help me out -- what is needed to get it over the finish line? Could I resurrect it to get that last bit done? |
|
@jamestelfer thanks for the prod, I'd forgotten to revisit this PR after the review from @lindell — let me do a quick rebase and then take a look at addressing the comments |
|
I now commit a change for the "update instead of add" change for github. I think that only Gitea reviewers are left now. |
|
@lindell thanks! I'll take a look at porting your github changes to the gitea pkg |
dnwe
force-pushed
the
replace-also-updates-pr
branch
4 times, most recently
from
7e8cdd8 to
926bde8
Compare
|
@lindell I've rebased on later master and I think this PR should be ready for a re-review now |
|
Note: for gitea, I spun up a docker image with an install and configured three sample users and tested that the replace of review requests was working 
|
Previous when using `--conflict-strategy replace` multi-gitter would replace all of the commits, but would leave the pull request itself untouched if it was already open. Now if multi-gitter is re-run and (e.g.,) the pr-title, pr-body or labels are changed, then the existing PR will be edited and updated to use their new values
dnwe
force-pushed
the
replace-also-updates-pr
branch
from
aa04e9d to
fe9adb2
Compare
…bels Instead of just adding them, the replace strategy will now overwrite them by doing any necessary add and removes
When using replace strategy, replace the review requests where they differ and remove any that should no longer apply
Now that multi-gitter has a baseline of Go 1.21 we can switch to using stdlib for the slices package. Note: we still need golang.org/x/exp/maps for maps.Values, which is not yet available in the equivalent stdlib maps package and likely won't be until Go 1.23
If no new reviewers were passed as arguments then we just leave the existing ones alone. Also simplify by switching to using non-interface repository for the param as setReviewers is an internal func to this package.
dnwe
force-pushed
the
replace-also-updates-pr
branch
from
fe9adb2 to
93f28c4
Compare
|
Included in release v0.50.0 🎉 |
What does this change
Previous when using
--conflict-strategy replacemulti-gitter would replace all of the commits, but would leave the pull request itself untouched if it was already open.Now if multi-gitter is re-run and (e.g.,) the pr-title, pr-body or labels are changed, then the existing PR will be edited and updated to use their new values
Checklist