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

PeepholePermalink RunListenerImpl oncompleted should be triggered before downstream builds are triggered

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Major Major
    • core
    • None

      Scenario
      ProjectA triggers ProjectB
      ProjectB has a RunParameter for ProjectA with a filter set for Successful Builds Only. The Run Parameter uses the PeepholePermalink to determine the LastSuccessful build.

      When ProjectA completes successfully, it will trigger a build of ProjectB before the PeepholePermalink cache is updated. This causes the RunParameter to default to the previous successful build of ProjectA, not the one which has just completed.

          [JENKINS-20989] PeepholePermalink RunListenerImpl oncompleted should be triggered before downstream builds are triggered

          Geoff Cummings created issue -
          Geoff Cummings made changes -
          Link New: This issue is related to JENKINS-20857 [ JENKINS-20857 ]

          ikedam added a comment -

          It seems that you need an access not to the latest build, but to the build that triggered the downstream build.
          If so, I know it can be resolved by adding RunParameter support to parameterized build plugin, but you may also consider using UpstreamCause to access to the upstream build.
          For example, Copy Artifact plugin provides an option to retrieve artifacts from the upstream build that triggered that downstream build.

          ikedam added a comment - It seems that you need an access not to the latest build, but to the build that triggered the downstream build. If so, I know it can be resolved by adding RunParameter support to parameterized build plugin, but you may also consider using UpstreamCause to access to the upstream build. For example, Copy Artifact plugin provides an option to retrieve artifacts from the upstream build that triggered that downstream build.

          Some of the downstream builds might be configured for All, lastCompleted, lastSuccessful and others for lastStable. Multiple downstream builds might have different requirements. It would be better to have this config in one place, the downstream jobs, and not have to work around this change and require additional logic in the upstream build to determine what RunParameter to pass to the downstream job.

          It worked as expected when the Run#getLastStableBuild used to be like:

              public RunT getLastStableBuild() {
                  RunT r = getLastBuild();
                  while (r != null
                          && (r.isBuilding() || r.getResult().isWorseThan(Result.SUCCESS)))
                      r = r.getPreviousBuild();
                  return r;
              }
          

          It no longer works after it was changed to use the Permalink

              public RunT getLastStableBuild() {
                  return (RunT)Permalink.LAST_STABLE_BUILD.resolve(this);
              }
          

          The State.POST_PRODUCTION seems to be for this very scenario, where the build result is now final and it is now ok to trigger other builds as described in JENKINS-980

          private static enum State {
              /**
               * Build is completed now, and the status is determined,
               * but log files are still being updated.
               *
               * The significance of this state is that Jenkins
               * will now see this build as completed. Things like
               * "triggering other builds" requires this as pre-condition.
               * See JENKINS-980.
               */
              POST_PRODUCTION,
          

          In the Run#execute method, I think we should update the PeepholePermalinks as soon as we change state and before we trigger fireCompleted on the listeners????
          Currently the Permalinks are updated in a fireCompleted event on a listener, the same as triggering downstream jobs. But they seem to be updated after the other builds are triggered.

              // advance the state.
              // the significance of doing this is that Jenkins
              // will now see this build as completed.
              // things like triggering other builds requires this as pre-condition.
              // see issue #980.
              //
              state = State.POST_PRODUCTION;
          
              if (listener != null) {
                  try {
                      job.cleanUp(listener);
                  } catch (Exception e) {
                      handleFatalBuildProblem(listener,e);
                      // too late to update the result now
                  }
                  RunListener.fireCompleted(this,listener);
                  listener.finished(result);
                  listener.closeQuietly();
              }
          

          Geoff Cummings added a comment - Some of the downstream builds might be configured for All, lastCompleted, lastSuccessful and others for lastStable. Multiple downstream builds might have different requirements. It would be better to have this config in one place, the downstream jobs, and not have to work around this change and require additional logic in the upstream build to determine what RunParameter to pass to the downstream job. It worked as expected when the Run#getLastStableBuild used to be like: public RunT getLastStableBuild() { RunT r = getLastBuild(); while (r != null && (r.isBuilding() || r.getResult().isWorseThan(Result.SUCCESS))) r = r.getPreviousBuild(); return r; } It no longer works after it was changed to use the Permalink public RunT getLastStableBuild() { return (RunT)Permalink.LAST_STABLE_BUILD.resolve( this ); } The State.POST_PRODUCTION seems to be for this very scenario, where the build result is now final and it is now ok to trigger other builds as described in JENKINS-980 private static enum State { /** * Build is completed now, and the status is determined, * but log files are still being updated. * * The significance of this state is that Jenkins * will now see this build as completed. Things like * "triggering other builds" requires this as pre-condition. * See JENKINS-980. */ POST_PRODUCTION, In the Run#execute method, I think we should update the PeepholePermalinks as soon as we change state and before we trigger fireCompleted on the listeners???? Currently the Permalinks are updated in a fireCompleted event on a listener, the same as triggering downstream jobs. But they seem to be updated after the other builds are triggered. // advance the state. // the significance of doing this is that Jenkins // will now see this build as completed. // things like triggering other builds requires this as pre-condition. // see issue #980. // state = State.POST_PRODUCTION; if (listener != null ) { try { job.cleanUp(listener); } catch (Exception e) { handleFatalBuildProblem(listener,e); // too late to update the result now } RunListener.fireCompleted( this ,listener); listener.finished(result); listener.closeQuietly(); }
          Geoff Cummings made changes -
          Link New: This issue is related to JENKINS-980 [ JENKINS-980 ]
          Jesse Glick made changes -
          Link New: This issue is related to JENKINS-16023 [ JENKINS-16023 ]

          Jesse Glick added a comment -

          Makes sense, file a pull request with a test.

          Jesse Glick added a comment - Makes sense, file a pull request with a test.

          I am also interested in a fix for this. I've just forked the jenkins repo and I'm still trying to get my head around the project setup.

          Horia Constantin added a comment - I am also interested in a fix for this. I've just forked the jenkins repo and I'm still trying to get my head around the project setup.
          Geoff Cummings made changes -
          Link New: This issue is related to JENKINS-22800 [ JENKINS-22800 ]
          Oliver Gondža made changes -
          Link New: This issue is duplicated by JENKINS-27635 [ JENKINS-27635 ]

            olivergondza Oliver Gondža
            gcummings Geoff Cummings
            Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

              Created:
              Updated:
              Resolved: