huserben / TfsExtensions

Extensions for TFS 2015+ such as custom Widgets (require TFS 2017) and Build Tasks
MIT License
45 stars 22 forks source link

Consider option for blocking against `inProgress` builds #46

Closed cbaxter closed 6 years ago

cbaxter commented 6 years ago

Perhaps an atypical use case, but I'd be interested in adding support for treating inProgress builds the same as notStarted when checking the Enable Build In Queue Condition status. We are using the trigger build task to support cascading builds in our build process and we have a few cases where two different build definitions may result in a common down-stream build being triggered (not ideal, but they exist).

As such, I was hoping to be able to prevent queuing a new build if another build that would trigger the same build is already in the queue or currently running to ensure the build isn't triggered more than once. Since TFS tends to display queued/running builds in the same list, this may not be a completely absurd idea... but I'd appreciate feedback on whether or not such a change would be welcome?

Thinking about changing the following:

https://github.com/huserben/TfsExtensions/blob/08a8257f677d6a8b2f27e539e68988b54b3e9ceb/BuildTasks/powershellTask/triggerbuildtask/triggerbuild.ps1#L245

To something along the lines of:

$queuedBuilds = Get-Build-By-Status -buildDefinitionId $blockingBuildId -statusFilter "notStarted
if ($queuedBuilds.Count -ne 0){
    Write-Output "$($_) is queued - will not trigger new build"
    exit
}

if($includeInProgressBuildsAsBlocking){
   $runningBuilds = Get-Build-By-Status -buildDefinitionId $blockingBuildId -statusFilter "inProgress"
   if ($runningBuilds.Count -ne 0){
        Write-Output "$($_) is in progress - will not trigger new build"
        exit
    }
}

NOTE: May be a cleaner way of doing this... just an initial "thought" without much investigation

An alternate (and potentially more flexible) option would be to allow the statusFilter value(s) to be configurable with a default of notStarted for backward compatibility...

huserben commented 6 years ago

Hi @cbaxter

thanks for the input. I think it's not totally far fetched to include at least an option for builds in progress.

However i see you added your samples for the PowerShell script, are you still using version 1 of the Task? Since version 2 is a complete rewrite in typescript the PowerShell version is not really maintained anymore and as well not offered via the marketplace. If you are not using version 2 or you are not able to switch to this version and you have to remain on the PowerShell task, I could assist you in achieving the feature but it won't be able to make to it to the marketplace. Otherwise I look at it as a new feature to be done in the next update of the typescript version.

cbaxter commented 6 years ago

@huserben Oversight on my part, I am using the v2 task from the marketplace...

Given the following...

https://github.com/huserben/TfsExtensions/blob/f6badc7d0f1ddb2d551aaa75819e6701c07a2423/BuildTasks/triggerbuildtask/taskrunner.ts#L132

A crude conversion to typescript of the rough PoC snippet above...

let queuedBuilds: tfsService.IBuild[]
    = await this.tfsRestService.getBuildsByStatus(blockingBuild, `${tfsService.BuildStateNotStarted}`);

if (queuedBuilds.length > 0) {
    console.log(`${blockingBuild} is queued - will not trigger new build.`);

    return false;
}

if(this.includeInProgressBuildsAsBlocking) {
    let queuedBuilds: tfsService.IBuild[]
        = await this.tfsRestService.getBuildsByStatus(blockingBuild, `${tfsService.BuildStateInProgress}`);

    if (queuedBuilds.length > 0) {
        console.log(`${blockingBuild} is in progress - will not trigger new build.`);

        return false;
    }
}
huserben commented 6 years ago

Hi

ok thats good, because the new task is way easier to extend with new functionality. I extended now the configuration so that there is a checkbox (default value: false) to include in progress builds for the specified blocking builds: grafik If enabled it does exactly this grafik

Would this be acceptable for you? I think there is no need to make it more configurable (e.g. to be able to filter for other states). You could theoretically put a textfield and let the user specify the states as comma separated values, but I that is then more prone to error.

What do you think?

cbaxter commented 6 years ago

@huserben This will work perfectly for my needs! I agree that this additional option is likely sufficient and further customization isn't warranted... I cannot think of any reason why you would want to block a build on Completed or Cancelling... perhaps Postponed is a little more interesting in that the status indicates the build is inactive in the queue. To be honest, I am not sure when a build is actually placed in this status (perhaps when no available build agents meet the required demands?) as I have never seen it myself...

huserben commented 6 years ago

Yes I was thinking the same - as well I don't really know when Postponed will happen. I think for now I will leave it with this option, and in case someone requests another option I can still enable a bit more flexible approach, but I would not like to increase the configuration effort too much.

In that case I will upload a new version of the task later (i'll update the post once its done).

Please let me then know if everything works fine so I can either check whats wrong or close the issue :-)

Edit: Version 2.5 that includes the additional option is now uploaded

cbaxter commented 6 years ago

@huserben Excellent, thank you for the quick response and turnaround! I will test the updated version later this afternoon and confirm that everything is working as expected.

Edit: Another project chewed up my entire day, will verify this tomorrow morning.

cbaxter commented 6 years ago

Initial results look promising, however I am seeing an unexpected behavior that may require attention... specifically, the Include current Build Definition as blocking Build option does not play nice with the new Include builds that are currently in progress option. The former option makes sense for builds that are notStarted, however will result in builds always being blocked if the latter option is also enabled.

Question Should the Include current Build Definition as blocking Build only apply to the notStarted build check?

Workaround I can uncheck the Include current Build Definition as blocking Build option, and then things will work for my use case, although that will result in queued instances of the build not act as blocking builds... lesser of two evils, but not ideal... input appreciated.


AdminTools.master Build Trigger Definition

image

Log Output 2018-01-28T15:04:14.8683038Z Using same branch as source version: refs/heads/master 2018-01-28T15:04:14.8683038Z Build in Queue Condition is enabled 2018-01-28T15:04:14.8683038Z Current Build Definition shall be included 2018-01-28T15:04:14.8683038Z Will treat in progress builds as blocking. 2018-01-28T15:04:14.8683038Z Following builds are blocking: 2018-01-28T15:04:14.8683038Z ManagementOfChangeBridge.master 2018-01-28T15:04:14.8683038Z AdminTools.master 2018-01-28T15:04:14.8683038Z Checking if blocking builds are queued 2018-01-28T15:04:14.8683038Z Checking build ManagementOfChangeBridge.master 2018-01-28T15:04:14.9776836Z Checking build AdminTools.master 2018-01-28T15:04:15.0089338Z AdminTools.master is queued - will not trigger new build. 2018-01-28T15:04:15.0089338Z ##[section]Finishing: Trigger builds


ManagementOfChange.master Build Trigger Definition

image

Log Output 2018-01-28T15:03:47.6572779Z Using same branch as source version: refs/heads/master 2018-01-28T15:03:47.6572779Z Build in Queue Condition is enabled 2018-01-28T15:03:47.6572779Z Current Build Definition shall be included 2018-01-28T15:03:47.6572779Z Will treat in progress builds as blocking. 2018-01-28T15:03:47.6572779Z Following builds are blocking: 2018-01-28T15:03:47.6572779Z AdminTools.master 2018-01-28T15:03:47.6572779Z ManagementOfChangeBridge.master 2018-01-28T15:03:47.6572779Z Checking if blocking builds are queued 2018-01-28T15:03:47.6572779Z Checking build AdminTools.master 2018-01-28T15:03:47.7822804Z AdminTools.master is queued - will not trigger new build. 2018-01-28T15:03:47.7978763Z ##[section]Finishing: Trigger builds

huserben commented 6 years ago

Hi

yes this does indeed not really make sense. I'm quickly adjusting the logic to exclude the current build definition if it is checked and for this one still only check the "IsQueued" state.

Thanks for that finding, I wasn't thinking about that case.

I'll update you once it's online (should be a quick thing to fix though :-))

huserben commented 6 years ago

So I updated the description of the task and the documentation on displayed on the marketplace with the exception: grafik

As well I added a log message for this use case: grafik

I'll finish up the code and update you once the version is uploaded

Version 2.5.1 with the mentioned change is now online

cbaxter commented 6 years ago

Will confirm updated behavior tomorrow morning; unable to verify this afternoon. Thanks again for the quick turnaround!

cbaxter commented 6 years ago

Everything appears to be working as expected now; I will continue to monitor builds throughout the day, but considering this issue closed; thanks again!

huserben commented 6 years ago

Ok perfect. If you have any other inputs or find something that seems not to work as expected please feel free to raise a new issue.