Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-49897

gerrit-plugin pr branch names are incorrectly name

      It seems that there are some naming problem with how gerrit-plugin is exposing the PR branches to Jenkins.

      A) On BlueOcean UI PR branches are named "C-123456/2" which is problematic for two reasons: string is too long to be correctly displayed in UI, which allows only ~6 chars.

      B) BlueOcean: exposed branch name exposes internals of gerrit implementation of temporary branches which is useless to the user "56/123456/2". 56/ prefix is only the last part of the original changeset and used for hashing folder name. last number is the version of the patchset. But from the logical point of view a PR/CR should be a single branch, regardless how many patchsets are build inside it. It makes sense to have a numeric only branch name, like "123456".

      C) On Classic UI, the PR branch is recognised as a normal branch and not as a PR. The PR tab is not created. Clearly the Classic UI is able to display PR separately because they do this correctly for GitHub based branch sources. Even more funny is that if someone configures two SCM sources on the same multibrach project, one as Gerrit and one as GitHub, the display of PR/CR is made correct. This means that simple existence of GitHub SCM branch make the multibranch display them correctly.

       

       

          [JENKINS-49897] gerrit-plugin pr branch names are incorrectly name

          Thanks for reporting the issue: it doesn't exactly classify as a bug IMHO. However, yes, the branch name is quite misleading.

          With regards to the fact that the Gerrit change-id is an internal implementation of the branch, yes I agree. However, GitHub PRs are displayed with the PR number, which is also an internal implementation of the branch.

          What I could drop is the initial prefix, which is only a namespace partitioning done by Gerrit to allow a faster execution. 

          Last but not least, the path number suffix. That one is REALLY NEEDED because it represents what you've build. If I want to retrigger a build of a patch-set, I need that information because I want to be sure of what I am building.

          Gerrit changes are very different from GitHub PRs from that point of view.

          ssbarnea what do you think?

          Luca Domenico Milanesio added a comment - Thanks for reporting the issue: it doesn't exactly classify as a bug IMHO. However, yes, the branch name is quite misleading. With regards to the fact that the Gerrit change-id is an internal implementation of the branch, yes I agree. However, GitHub PRs are displayed with the PR number, which is also an internal implementation of the branch. What I could drop is the initial prefix, which is only a namespace partitioning done by Gerrit to allow a faster execution.  Last but not least, the path number suffix. That one is REALLY NEEDED because it represents what you've build. If I want to retrigger a build of a patch-set, I need that information because I want to be sure of what I am building. Gerrit changes are very different from GitHub PRs from that point of view. ssbarnea what do you think?

          Jose Sa added a comment -

          I also consider that we should only consider the gerrit review id (6 digits number) as the branch name, and treat it the same way as the other actual branches.

          This way all individual patches, manual replays, etc... will be displayed in the history of that branch, retaining a build history while the review is open.

          As it is right now, once a new patchset is uploaded previous related builds are destroyed and only the latest patchset "job" is available, effectively loosing history and for me that is a serious bug.

          Jose Sa added a comment - I also consider that we should only consider the gerrit review id (6 digits number) as the branch name, and treat it the same way as the other actual branches. This way all individual patches, manual replays, etc... will be displayed in the history of that branch, retaining a build history while the review is open. As it is right now, once a new patchset is uploaded previous related builds are destroyed and only the latest patchset "job" is available, effectively loosing history and for me that is a serious bug.

          The multi-branch pipeline can be configured to keep the obsolete branches (patch-sets not up to date) for a number of days. Would that resolve your issue?

          Luca Domenico Milanesio added a comment - The multi-branch pipeline can be configured to keep the obsolete branches (patch-sets not up to date) for a number of days. Would that resolve your issue?

          I would also vote for having only the Change number field as the PR name. Having the Patchset number as part of the branch/PR makes Jenkins forget the history of the change. All PRs only get one build because Patchsets are seen as different PRs, so the statistics provided by plugins such as Warnings are meaningless.

          I don't really buy the motivation for the Patchset number quoted above; that it's needed to retrigger a build of a Patchset. If a newer Patchset is uploaded, it obsoletes all previous Patchsets of that Change. They cannot be submitted, so what is the point of building them at all? In fact in a comment to JENKINS-60301, it's even discussed to abort ongoing builds of older Patchsets when a new one is uploaded. Besides, since the job for the current Patchset is removed when a new is uploaded, you can't even retrigger a build.

          Andreas Fritiofson added a comment - I would also vote for having only the Change number field as the PR name. Having the Patchset number as part of the branch/PR makes Jenkins forget the history of the change. All PRs only get one build because Patchsets are seen as different PRs, so the statistics provided by plugins such as Warnings are meaningless. I don't really buy the motivation for the Patchset number quoted above; that it's needed to retrigger a build of a Patchset. If a newer Patchset is uploaded, it obsoletes all previous Patchsets of that Change. They cannot be submitted, so what is the point of building them at all? In fact in a comment to  JENKINS-60301 , it's even discussed to abort ongoing builds of older Patchsets when a new one is uploaded. Besides, since the job for the current Patchset is removed when a new is uploaded, you can't even retrigger a build.

          What would be the point of re-triggering a build for an obsolete patch-set?

          Can you point me to the statistics that would be useful if we would keep the same job for the same change? (URL of the feature you would have the associated benefits?)

           

          Luca Domenico Milanesio added a comment - What would be the point of re-triggering a build for an obsolete patch-set? Can you point me to the statistics that would be useful if we would keep the same job for the same change? (URL of the feature you would have the associated benefits?)  

          Any more comments on this?

          Luca Domenico Milanesio added a comment - Any more comments on this?

          What would be the point of re-triggering a build for an obsolete patch-set?

          I have no idea, it sounded like you wanted to do it in your first comment above. I don't want to do it, and it can't even be done. So let's forget about that. What I now realize you meant in that comment was that you think the PatchSet number in the branch name is important to be able to manually verify (e.g. when re-triggering a build) that Jenkins is up-to-date about which is the latest PatchSet in Gerrit. So you don't accidentally trigger a build before Jenkins has realized there's a new PatchSet and you end up with a build of an obsolete PatchSet. Sounds like a valid point, although I guess the commit SHA1 is a stronger check for that.

          The problem I had (and have) with treating each PatchSet as a fresh new job is that you don't get the nice statistics the Jenkins provide across the build numbers (since there will only ever be one build per job). This also affects other plugins that show statistics, such as Warnings. You cannot see how a Change has evolved. This of course assumes that the branch names being unique per PatchSet instead of Change is the only issue that prevent the statistics from working. If you point me to where the names are determined in the plugin source code, I can build a custom version and see if it actually does help in this case.

          If we look at the original bug report it has actually four parts:

          A) On BlueOcean UI PR branches are named "C-123456/2" which is problematic for two reasons: string is too long to be correctly displayed in UI, which allows only ~6 chars.

          This is still the current naming, however I'm not sure if BlueOcean still has the ~6 char limitation. I don't have so many changes... I think if this is still a problem it should be fixed in BlueOcean, right?

          By the way, what is the motivation for the C- prefix, does it mean anything? Why is it different from the name each change has in the Classic UI? Even within BlueOcean, if you click on a PR C-12345/6, the header then says Pull Request: 45/12345/6 (like the Classic name), so not even consistent. What is the name of the Pull request? It seems there are two different and unless that's a feature, it should be changed.

          B) BlueOcean: exposed branch name exposes internals of gerrit implementation of temporary branches which is useless to the user "56/123456/2". 56/ prefix is only the last part of the original changeset and used for hashing folder name. last number is the version of the patchset.

          Yeah, this is just pointless and should be removed, the name should be just 12345, or 12345/6, whichever is chosen. Surely not 45/12345/6. This is not only for BlueOcean, the Classic change name is also on this format.

          But from the logical point of view a PR/CR should be a single branch, regardless how many patchsets are build inside it. It makes sense to have a numeric only branch name, like "123456".

          This could be discussed, I'm not sure of all the consequences of either choice.

          C) On Classic UI, the PR branch is recognised as a normal branch and not as a PR. The PR tab is not created.

          This seems to work now.

          Andreas Fritiofson added a comment - What would be the point of re-triggering a build for an obsolete patch-set? I have no idea, it sounded like you wanted to do it in your first comment above. I don't want to do it, and it can't even be done. So let's forget about that. What I now realize you meant in that comment was that you think the PatchSet number in the branch name is important to be able to manually verify (e.g. when re-triggering a build) that Jenkins is up-to-date about which is the latest PatchSet in Gerrit. So you don't accidentally trigger a build before Jenkins has realized there's a new PatchSet and you end up with a build of an obsolete PatchSet. Sounds like a valid point, although I guess the commit SHA1 is a stronger check for that. The problem I had (and have) with treating each PatchSet as a fresh new job is that you don't get the nice statistics the Jenkins provide across the build numbers (since there will only ever be one build per job). This also affects other plugins that show statistics, such as Warnings . You cannot see how a Change has evolved. This of course assumes that the branch names being unique per PatchSet instead of Change is the only issue that prevent the statistics from working. If you point me to where the names are determined in the plugin source code, I can build a custom version and see if it actually does help in this case. If we look at the original bug report it has actually four parts: A) On BlueOcean UI PR branches are named "C-123456/2" which is problematic for two reasons: string is too long to be correctly displayed in UI, which allows only ~6 chars. This is still the current naming, however I'm not sure if BlueOcean still has the ~6 char limitation. I don't have so many changes... I think if this is still a problem it should be fixed in BlueOcean, right? By the way, what is the motivation for the C- prefix, does it mean anything? Why is it different from the name each change has in the Classic UI? Even within BlueOcean, if you click on a PR C-12345/6, the header then says Pull Request: 45/12345/6 (like the Classic name), so not even consistent. What is the name of the Pull request? It seems there are two different and unless that's a feature, it should be changed. B) BlueOcean: exposed branch name exposes internals of gerrit implementation of temporary branches which is useless to the user "56/123456/2". 56/ prefix is only the last part of the original changeset and used for hashing folder name. last number is the version of the patchset. Yeah, this is just pointless and should be removed, the name should be just 12345, or 12345/6, whichever is chosen. Surely not 45/12345/6. This is not only for BlueOcean, the Classic change name is also on this format. But from the logical point of view a PR/CR should be a single branch, regardless how many patchsets are build inside it. It makes sense to have a numeric only branch name, like "123456". This could be discussed, I'm not sure of all the consequences of either choice. C) On Classic UI, the PR branch is recognised as a normal branch and not as a PR. The PR tab is not created. This seems to work now.

          Sten Olsson added a comment - - edited

          In regard to A) - The name with "C-" appears to be passed to Jenkins and thus the pipeline as environment variable "CHANGE_ID".  I guess this comes from the gerrit-code-review plugin, but maybe from the multibranch pipeline instead somehow? 

          With B) - Blue Ocean doesn't handle the link names correctly when builds are triggered with gerrit-code-plugin.  I often have to manually change the URL after getting a 404 error.

          And C) - for me, the PR tab is created on Blue Ocean and "Changes" tab on Classic.  Maybe it is because I'm using

           refs/changes/*:refs/changes/*

          as my refspec or that shouldnt' matter?

          My opinion is that it's a good idea to try to "do what others are doing" in regard to how other plugins like GitHub work.  This means future integration comes more "for free" instead of custom code.  In the past I learned my lesson trying to customize Jira to work just like our other issue tool and ended up with a Frankenstein.  I'm not saying gerrit-code-review would become like that, but I just mean that maybe it's better to do as others do or at least make it configurable.  I'm guessing it may even fix the Blue Ocean plugin not generating URLs correctly and other things (like ability to easily kill prior patchset jobs without resorting to Jenkins API calls). 

          Sten Olsson added a comment - - edited In regard to A) - The name with "C-" appears to be passed to Jenkins and thus the pipeline as environment variable "CHANGE_ID".  I guess this comes from the gerrit-code-review plugin, but maybe from the multibranch pipeline instead somehow?  With B) - Blue Ocean doesn't handle the link names correctly when builds are triggered with gerrit-code-plugin.  I often have to manually change the URL after getting a 404 error. And C) - for me, the PR tab is created on Blue Ocean and "Changes" tab on Classic.  Maybe it is because I'm using refs/changes/*:refs/changes/* as my refspec or that shouldnt' matter? My opinion is that it's a good idea to try to "do what others are doing" in regard to how other plugins like GitHub work.  This means future integration comes more "for free" instead of custom code.  In the past I learned my lesson trying to customize Jira to work just like our other issue tool and ended up with a Frankenstein.  I'm not saying gerrit-code-review would become like that, but I just mean that maybe it's better to do as others do or at least make it configurable.  I'm guessing it may even fix the Blue Ocean plugin not generating URLs correctly and other things (like ability to easily kill prior patchset jobs without resorting to Jenkins API calls). 

          Joe Hansche added a comment -

          Another unfortunate side effect of this naming convention is how Jenkins links are built:

          https://jenkins.gerritforge.com/job/gerrit-code-review-plugin/job/53%252F500053%252F3/1/

          It is not easy at all, to see what change or patchset number this is for. The branch name gets double-url-encoded: "/" -> "%2F" -> "%252F". 

          Joe Hansche added a comment - Another unfortunate side effect of this naming convention is how Jenkins links are built: https://jenkins.gerritforge.com/job/gerrit-code-review-plugin/job/53%252F500053%252F3/1/ It is not easy at all, to see what change or patchset number this is for. The branch name gets double-url-encoded: "/" -> "%2F" -> "%252F". 

            lucamilanesio Luca Domenico Milanesio
            ssbarnea Sorin Sbarnea
            Votes:
            4 Vote for this issue
            Watchers:
            6 Start watching this issue

              Created:
              Updated: