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

Copied jobs should be disabled by default

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Component/s: core
    • Labels:
      None
    • Environment:
      Platform: All, OS: All

      Description

      When you copy a job it should be disabled by default so that any modifications
      can be made to it before it tries to run.

      1. copied_job_patch.diff
        2 kB
        jpederzolli
      2. projectCopy.patch
        0.6 kB
        ajpurkiss

        Issue Links

          Activity

          Hide
          ajpurkiss ajpurkiss added a comment -

          Created an attachment (id=411)
          Patch to disable the job

          Show
          ajpurkiss ajpurkiss added a comment - Created an attachment (id=411) Patch to disable the job
          Hide
          torbent torbent added a comment -

          Absolutely! I've been bitten by this many times, sadly. The problem is made
          worse in our setup because we use custom workspaces, so the new copy actually
          builds on top of the original. Unless I'm quick to disable the new job.
          Maintainers, there's already a (year-old) patch to fix this issue, so can we
          have it fixed, please?

          Show
          torbent torbent added a comment - Absolutely! I've been bitten by this many times, sadly. The problem is made worse in our setup because we use custom workspaces, so the new copy actually builds on top of the original. Unless I'm quick to disable the new job. Maintainers, there's already a (year-old) patch to fix this issue, so can we have it fixed, please?
          Hide
          torbent torbent added a comment -

          (added me as cc)

          Show
          torbent torbent added a comment - (added me as cc)
          Hide
          mdillon mdillon added a comment -

          We had a ticket in our (Yahoo!'s) internal tracker for this same thing. Here is
          the comment I made there:

          Here's the rough sequence for a clone:

          1. Make an in memory copy of the job to be cloned
          2. Change the name on that copy
          3. Add the copy to Hudson's internal job list
          4. Save the job to disk
          5. Redirect to the post configuration screen

          The sequence for a new job is about the same except it starts with a blank copy
          of the job.

          In terms of implementation, it may be that we could do this:

          2a. Set disabled flag to true
          ...
          5a. Set disabled flag in the HTML form to whatever it was in the original job

          This way, when someone saves it will be enabled without them having to remember
          to check the checkbox, but Hudson won't actually see an enabled job unless it is
          saved. This behavior would only happen on the post config redirect; if you
          abandon the post config page, the job stays disabled and if you edit it, the
          form will show it as disabled.

          Show
          mdillon mdillon added a comment - We had a ticket in our (Yahoo!'s) internal tracker for this same thing. Here is the comment I made there: Here's the rough sequence for a clone: 1. Make an in memory copy of the job to be cloned 2. Change the name on that copy 3. Add the copy to Hudson's internal job list 4. Save the job to disk 5. Redirect to the post configuration screen The sequence for a new job is about the same except it starts with a blank copy of the job. In terms of implementation, it may be that we could do this: 2a. Set disabled flag to true ... 5a. Set disabled flag in the HTML form to whatever it was in the original job This way, when someone saves it will be enabled without them having to remember to check the checkbox, but Hudson won't actually see an enabled job unless it is saved. This behavior would only happen on the post config redirect; if you abandon the post config page, the job stays disabled and if you edit it, the form will show it as disabled.
          Hide
          mdillon mdillon added a comment -

          Can't believe this thing makes me add a comment to put myself on the cc list...
          Can't wait for JIRA.

          Show
          mdillon mdillon added a comment - Can't believe this thing makes me add a comment to put myself on the cc list... Can't wait for JIRA.
          Hide
          ajpurkiss ajpurkiss added a comment -

          Interesting, though from my point of view I have no problem either unchecking
          the box before I save or clicking the enable button when the job is saved.

          Show
          ajpurkiss ajpurkiss added a comment - Interesting, though from my point of view I have no problem either unchecking the box before I save or clicking the enable button when the job is saved.
          Hide
          kohsuke Kohsuke Kawaguchi added a comment -

          When slaves are copied, the newly created one is put into a temporary transitive
          "disabled until next save" state. I wonder if the same should be done for jobs,
          instead of disabling the job, which would require people enabling it manually.

          Show
          kohsuke Kohsuke Kawaguchi added a comment - When slaves are copied, the newly created one is put into a temporary transitive "disabled until next save" state. I wonder if the same should be done for jobs, instead of disabling the job, which would require people enabling it manually.
          Hide
          mdillon mdillon added a comment -

          I hadn't seen holdOffLaunchUntilSave, but something that is basically what I was
          suggesting. That would let the "disabled" flag keep the original state of the
          copied job, while the other flag would make isDisabled() return true regardless
          of the state of the "disabled" field. The jelly form would need to be changed to
          look at a method other than isDisabled() to allow it to reflect the original
          state of the copied job until a save is done.

          Show
          mdillon mdillon added a comment - I hadn't seen holdOffLaunchUntilSave, but something that is basically what I was suggesting. That would let the "disabled" flag keep the original state of the copied job, while the other flag would make isDisabled() return true regardless of the state of the "disabled" field. The jelly form would need to be changed to look at a method other than isDisabled() to allow it to reflect the original state of the copied job until a save is done.
          Hide
          ajpurkiss ajpurkiss added a comment -

          Interesting, I like the idea of only a temporary disable (unless of course the
          user marks it disabled). My main concern right now is that polling jobs that
          are copied are risky right now as they can run before all relevant changes are
          made. If the full fix to address this is going to take some time to implement
          (and I dont have time to help try to implement it) then I would like the
          temporary fix put in place as I have stopped building Hudson locally now as I
          can do most of my changes through plugin bar this.

          Show
          ajpurkiss ajpurkiss added a comment - Interesting, I like the idea of only a temporary disable (unless of course the user marks it disabled). My main concern right now is that polling jobs that are copied are risky right now as they can run before all relevant changes are made. If the full fix to address this is going to take some time to implement (and I dont have time to help try to implement it) then I would like the temporary fix put in place as I have stopped building Hudson locally now as I can do most of my changes through plugin bar this.
          Hide
          jpederzolli jpederzolli added a comment -

          I am currently working on the fix for this which follows the same approach as slaves, i.e. the cloned job will be in a temporary disabled state until the initial save. Unlike the initial patch in comment 1, the actual enabled/disabled status will be carried over from the job being cloned.

          Once this has been completed and tested internally (Yahoo!), I will look into merging upstream.

          Show
          jpederzolli jpederzolli added a comment - I am currently working on the fix for this which follows the same approach as slaves, i.e. the cloned job will be in a temporary disabled state until the initial save. Unlike the initial patch in comment 1, the actual enabled/disabled status will be carried over from the job being cloned. Once this has been completed and tested internally (Yahoo!), I will look into merging upstream.
          Hide
          jpederzolli jpederzolli added a comment -

          Attaching patch which prevents a cloned build from running, via build trigger or manually, until the initial save.

          Show
          jpederzolli jpederzolli added a comment - Attaching patch which prevents a cloned build from running, via build trigger or manually, until the initial save.
          Hide
          mindless Alan Harder added a comment -

          looks pretty good to me.. 2 questions:
          1) should the variable be initialized to false somewhere?
          2) what if you copy a job and then leave the config screen w/o saving? is it ok to assume they'll come back and save it at some point to get it running? seems like this is ok, but thought i'd raise the question since there will be no indication why the job isn't running.. (I guess same question applies to copied slave nodes..)

          Show
          mindless Alan Harder added a comment - looks pretty good to me.. 2 questions: 1) should the variable be initialized to false somewhere? 2) what if you copy a job and then leave the config screen w/o saving? is it ok to assume they'll come back and save it at some point to get it running? seems like this is ok, but thought i'd raise the question since there will be no indication why the job isn't running.. (I guess same question applies to copied slave nodes..)
          Hide
          jpederzolli jpederzolli added a comment -

          1) should the variable be initialized to false somewhere?

          The boolean variable is going to implicitly initialized to false, so I don't think this is required (though wouldn't hurt).

          2) what if you copy a job and then leave the config screen w/o saving? is it ok to assume they'll come back and save it at some point to get it running? seems like this is ok, but thought i'd raise the question since there will be no indication why the job isn't running.. (I guess same question applies to copied slave nodes..)

          This is a fair point; my initial thoughts were that a) this is consistent with the copied slave behavior and b) I think most users would not want a duplicate job to run until changes are made.

          With these changes, a cloned build that is not saved will show up as gray in the UI indicating it is disabled. This will at least let the user know that a build is not going to take place, but may leave them wondering why.

          Show
          jpederzolli jpederzolli added a comment - 1) should the variable be initialized to false somewhere? The boolean variable is going to implicitly initialized to false, so I don't think this is required (though wouldn't hurt). 2) what if you copy a job and then leave the config screen w/o saving? is it ok to assume they'll come back and save it at some point to get it running? seems like this is ok, but thought i'd raise the question since there will be no indication why the job isn't running.. (I guess same question applies to copied slave nodes..) This is a fair point; my initial thoughts were that a) this is consistent with the copied slave behavior and b) I think most users would not want a duplicate job to run until changes are made. With these changes, a cloned build that is not saved will show up as gray in the UI indicating it is disabled. This will at least let the user know that a build is not going to take place, but may leave them wondering why.
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in hudson
          User: : jpederzolli
          Path:
          trunk/hudson/main/core/src/main/java/hudson/model/AbstractProject.java
          trunk/hudson/main/core/src/main/java/hudson/model/Job.java
          http://jenkins-ci.org/commit/31444
          Log:
          Issue: JENKINS-2494

          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in hudson User: : jpederzolli Path: trunk/hudson/main/core/src/main/java/hudson/model/AbstractProject.java trunk/hudson/main/core/src/main/java/hudson/model/Job.java http://jenkins-ci.org/commit/31444 Log: Issue: JENKINS-2494
          Hide
          jpederzolli jpederzolli added a comment -

          committed patch after getting ok from Kohsuke.

          Show
          jpederzolli jpederzolli added a comment - committed patch after getting ok from Kohsuke.
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in hudson
          User: : mindless
          Path:
          trunk/www/changelog.html
          http://jenkins-ci.org/commit/31454
          Log:
          JENKINS-2494 note in change log

          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in hudson User: : mindless Path: trunk/www/changelog.html http://jenkins-ci.org/commit/31454 Log: JENKINS-2494 note in change log
          Hide
          nealeu nealeu added a comment -

          I've just copied a Maven job and found that it ran with the existing before I'd saved it.

          Has the "Apply" button behaviour caused a regression here?

          Show
          nealeu nealeu added a comment - I've just copied a Maven job and found that it ran with the existing before I'd saved it. Has the "Apply" button behaviour caused a regression here?
          Hide
          scm_issue_link SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: Stephen Connolly
          Path:
          core/src/main/java/hudson/model/Job.java
          http://jenkins-ci.org/commit/jenkins/503c3bd2e6f2ec85514e16a260396ddae68f03ae
          Log:
          [FIXES JENKINS-2494] Restore correct behaviour

          • Fixes a regression in core where the display name clear on copy was triggering a save
          • More than one way to do this, could also have used the marker interface approach
            This route seems slightly less fragile, though people could still add ItemListeners
            with order == -Double.MAX_VALUE which would then introduce intdeterminism.
            A marker interface would remove that indeterminism as the onCopyComplete method would
            be only called on the Job as the last method... but it could be hard to
            ensure that all ItemGroupMixin's respect the calling of onCopyComplete contract
            hence this approach seems better to me for that reason
          Show
          scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Stephen Connolly Path: core/src/main/java/hudson/model/Job.java http://jenkins-ci.org/commit/jenkins/503c3bd2e6f2ec85514e16a260396ddae68f03ae Log: [FIXES JENKINS-2494] Restore correct behaviour Fixes a regression in core where the display name clear on copy was triggering a save More than one way to do this, could also have used the marker interface approach This route seems slightly less fragile, though people could still add ItemListeners with order == -Double.MAX_VALUE which would then introduce intdeterminism. A marker interface would remove that indeterminism as the onCopyComplete method would be only called on the Job as the last method... but it could be hard to ensure that all ItemGroupMixin's respect the calling of onCopyComplete contract hence this approach seems better to me for that reason
          Hide
          dogfood dogfood added a comment -

          Integrated in jenkins_main_trunk #2687

          Result = UNSTABLE

          Show
          dogfood dogfood added a comment - Integrated in jenkins_main_trunk #2687 Result = UNSTABLE

            People

            • Assignee:
              jpederzolli jpederzolli
              Reporter:
              ajpurkiss ajpurkiss
            • Votes:
              4 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: