-
Improvement
-
Resolution: Unresolved
-
Minor
-
None
-
Powered by SuggestiMate
Instead of having a new "Change Request" in Multibranch Pipeline for every new patchset, it would be better to have just a single entry for the change, and new patchsets creating new builds into that already created "Change Request".
This way, we would be able to check the entire build history for a change request (currently every new patchset causes the old one to get discarded) and even check the Git diffs between the different patchsets, and most importantly: the arrangement.
- duplicates
-
JENKINS-49897 gerrit-plugin pr branch names are incorrectly name
-
- Open
-
- is duplicated by
-
JENKINS-68014 One "change" per change instead of per patch set
-
- Closed
-
- is related to
-
JENKINS-58958 Use the same Jenkins workspace on a Change basis
-
- Open
-
[JENKINS-62816] Group patchsets into a single change
Seems it's not as simple as I'd hoped, at first glance. There is additional code that relies on the branch name to match the ##/#####/# pattern. And there aren't any unit tests to verify what revision will get checked out in the checkout stage, so I don't feel comfortable just making the change blindly, as it looks like at least some other functionality will break
Thanks Joe. I limited the number of changes just now to 30 (previously I allowed 100 in Jenkins' Orphan Strategy) so that is manageable as it is.
One thing I still haven't implemented in my pipeline is killing prior patchset builds when a newer one is started. I know Gerrit Trigger does this, but I can't use Gerrit Trigger plugin due to how it communicates with Jenkins. Plus I think gerrit-code-review plugin is better in the long run as everything is done from the pipeline code. Pipeline's milestone will also do this, but your jobs have to be setup with a certain hierarchy for it to work (which doesn't seem so easy with Gerrit Code Review Plugin).
olssons there's a separate jira issue to track canceling a running build when a new patchset is uploaded: JENKINS-60301.
I did get the branch name to work, by fetching the original refspec into a locally renamed branch (fetching refspec: +refs/changes/45/12345/6:refs/remotes/origin/C-12345). I'll do some more testing and then prepare a change request for it.
This issue has been already discussed in the past with JENKINS-49897 and JENKINS-58958. Did you look at those discussion?
I know lots of people would like to see Gerrit change validations in a similar way they see GitHub PRs. However, the two models are different.
In Gerrit you always validate a single commit whilst in GitHub PRs you validate a series of commits in a branch.
The arguments of "being faster because of the reuse of the same workspace" are nowadays a bit obsolete, with build systems on the cloud and multi-agent Jenkins executors.
The last argument could be the "clarity of the number" visualised, which I recognise is a value. However, is it really so important?
Because this issue has been raised 3 TIMES (this is the 3rd iteration), I agree that IT IS important for lots of people. At the end of the day, we do OpenSource for the people that uses it, not for the author. Even if I believe it is not that important, I do acknowledge that it is important for other people, therefore I am more than happy to work on it.
If anyone has some initial code to share, fee free to create a WIP change to the [Gerrit Code Review plugin project on GerritHub.io|https://review.gerrithub.io/admin/repos/jenkinsci/gerrit-code-review-plugin,commands].
I will be more than happy to review, contribute and getting the feature merged.
lucamilanesio, thank you for the wealth of information!
with JENKINS-49897 and JENKINS-58958. Did you look at those discussion?
Sorry, no I did not look at the other issues, because I found an issue that describes exactly what I came here to ask for, and decided to comment on that instead of creating my own (like a good community netizen!). Unfortunately, it had not been triaged yet and linked to the duplicates that you pointed out, otherwise I would have followed up there instead.
As for the counter-arguments, I'll try to address those, at least from my point of view:
In Gerrit you always validate a single commit whilst in GitHub PRs you validate a series of commits in a branch.
Yes and no... I do agree that you are always only interested in the latest patchset of a change (as that is the only patchset that can be submitted to the project after it's approved). But I don't think the comparison to Github is relevant – I almost always amend my github PR commits, and force-push to my fork. Github is perfectly content accepting that, and everything works exactly as it should. There's no confusion, and it works kind of like Gerrit. Yes, you can push a series of commits to a PR branch, and Github gives the maintainer an option of how those will be merged into the project (either a simple merge commit, a rebase & fast-forward, or rebase and squash). Gerrit similarly gives you control over how the approved and submitted commit gets applied to the target branch.
What I do not agree with, however, is that a follow-up patchset from PS 1 to PS 2 and beyond, should ever in any way be treated as an entirely new "thing". Yes, it's a new commit, but there is a reason that Gerrit updates the existing change with just a new patchset: because patchset 2 is absolutely 100% tied to the original overall "change", and is not in any way intended to be completely discontiguous. Otherwise every time you make a followup commit, Gerrit would show the subsequent patchsets all side-by-side in the dashboard view (i.e., like it would if you do not have a Change-Id in your commit message, and you just keep amending and re-pushing).
By splitting the multi-branch pipeline change request "branch" builds into completely separate folders, they become discontiguous. You cannot see (easily) the evolution of the health of a change, from patchset to patchset. You can't see the history of that change. The only thing you can group together is subsequent re-executions of the exact same commit. Nothing else can be grouped inside this virtual folder that multibranch pipeline is literally designed for.
And following along those lines, the Multibranch pipeline plugin shows you the overall "health" of a given folder, by way of the "W" Weather Report column. By splitting every patchset into its own discontiguous folder, that weather report becomes less meaningful, because it can ONLY ever show the build results of a single git commit. It cannot show how patchset 1 failed to compile and then patchset 2 became unstable because of a unit test failure, and then patchset 3 is successful. Yes, you can find that information if you want to scan through every other commit, looking for all the ones that match your gerrit change number – but wouldn't it be more meaningful to have PS1, PS2, PS3 all grouped together under a single folder?
The arguments of "being faster because of the reuse of the same workspace" are nowadays a bit obsolete, with build systems on the cloud and multi-agent Jenkins executors.
I agree with you, reusing the same workspace is not a good reason for wanting this behavior. We use AWS EC2 slave instances anyway, so reusing a workspace is never a guarantee. This is not my reason for wanting this feature.
The last argument could be the "clarity of the number" visualised, which I recognise is a value. However, is it really so important?
I do think that this is the #1 reason, and I disagree with dismissing it by "is it really so important?" I think yes, it is very important, for the same reasons outlined above. Gerrit groups subsequent patchsets for a single change into a single change. There's a reason that it does that. Patchset 2 is related to Patchset 1, and should not (my opinion) be isolated into its own unrelated folder.
Because this issue has been raised 3 TIMES (this is the 3rd iteration), I agree that IT IS important for lots of people. At the end of the day, we do OpenSource for the people that uses it, not for the author. Even if I believe it is not that important, I do acknowledge that it is important for other people, therefore I am more than happy to work on it.
Thank you: I admire that you consider the community and realize that there are multiple points of view! I absolutely respect and appreciate all the work that you do, not only on this plugin, but in Gerrit as a whole. I got my company to convert to Gerrit in 2011, and we've never looked back!
If anyone has some initial code to share, fee free to create a WIP change
I was able to make incremental progress on getting it to show up as C-12345. But I kept running into other issues; as I would fix one issue, a new one would pop up. And I just wasn't familiar enough with the plugin to know the easiest way to get there. I will push what I have so far.
lucamilanesio I've opened a WIP change here: https://review.gerrithub.io/c/jenkinsci/gerrit-code-review-plugin/+/500053
Seeing as there seems to be a clear divide between people who want to see all patchsets for a change under one change-specific folder, vs people who want to see every change+patchset have its own folder, maybe we can find a way to make it configurable, so that i.e., the job can configure how it wants the branch name to be built:
- @{changeShard}/@{changeNumber}/@{patchNumber} - shows like it does today: "45/12345/6"
- @{changeNumber}/@{patchNumber} - shows like suggested in one of those linked tickets: "12345/6"
- C-@{changeNumber} - shows like what I'm suggesting here: "C-12345"
In order for this to work, there are two options:
- Have some way to associate the branch with the patch that is being built (this implies that you can build an arbitrary patchset that you choose, which may not be necessary)
- OR make the plugin always fetch the latest patchset. This would still work exactly the same way that other branch sources work... The others don't give you a way to "go back in time" and build an old (irrelevant) commit in a branch .. it just builds the branch, whatever the latest commit happens to be. Even the Github branch source (when PR is enabled), does not keep track of what commit (or even any info about the Pull Request itself, other than its number) associated with the "branch" as seen by the multibranch pipeline plugin. By only keeping track of the PR number and nothing else, the plugin will always make the Github API call to fetch information about that pull request number, and will always fetch the commit as of that time of the build.
I don't see why you would ever need to go back and rebuild an old patchset, so #1 doesn't really seem necessary to me.
I also added a comment on a recent commit https://review.gerrithub.io/c/jenkinsci/gerrit-code-review-plugin/+/499652 that fixes JENKINS-61107, which I think can help with this issue as well.
By associating that InvisibleAction with the Run, we can allow for that #1 option above, as now we'll be able to extract patchset information from an older build of an existing named branch. The way the action gets added would need to be parsed from the upstream ref for this to work, however, rather than from the local branch name – that is, parsed from upstream refs/changes/***, rather than local refs/remotes/origin/changes/***.
Thanks jhansche for starting this initiative: I always like different points of view and even more when they are accompanied with help in pushing this forward, so your help is highly appreciated.
Your change is half-working, there are still some TODOs (e.g. Environment variables contribution is broken) but we're getting there.
I don't think there will be value in making the branch scheme configurable: too much choice may create confusion. I believe that at the end of the day the purpose of CI validation is to score the change good or not for being merged. There is no value in scoring a patch that isn't the latest. Gerrit also discards any scoring non a non-latest patch-set anyway, so what's the point?
Using the approach of "asking Gerrit" what is the latest patch-set for that change would work.
Personally I do not really care so much about how the builds are organised in the multibranch pipeline folder, as long as they are linked by Gerrit checks tab. But I see your points and I acknowledge their importance for the community. So I am happy to move forward.
Let's continue the review on https://review.gerrithub.io/c/jenkinsci/gerrit-code-review-plugin/+/500053 and get that merged.
We'll have to upgrade the major version of the plugin (v0.5.0) as this is a major change. I believe it will be most welcomed by the majority of the users.
I'll open a PR to name the branch with just the change number, instead of the full ref, and see what Luca thinks.