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
    • Similar Issues:

      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.

        Attachments

          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: