Feature: Create builds directly instead of restarting old builds

[aknuds1]

This PR proposes a rewrite of the plugin to, instead of re-starting old builds, create builds via Client.BuildCreate.

[donny-dont]

@aknuds1 can you look at targeting the v2 branch?

[aknuds1]

@donny-dont Thanks, I wasn't aware.

[aknuds1]

Will have to fix it up later.

[aknuds1]

@donny-dont I pushed a revision now that's cleanly based on v2, where I retain the blocking functionality. Please have a look.

Also, would you mind explaining the purpose of the v2 branch, and its relation to the master branch?

[aknuds1]

@donny-dont Thanks! Will look into your feedback.

[donny-dont]

@aknuds1 with regards to the v2 branch we had this PR https://github.com/drone-plugins/drone-downstream/pull/70 which added a block option. Per https://github.com/drone-plugins/drone-downstream/issues/68 the ideal behavior was for that block that was added to be the wait option. The PR adding the params for the BuildCreate method in drone-go wasn't present yet so we merged things over into a v2 branch.

Also remember that drone-downstream is a very popular plugin so we'd need to have a proper migration path for users hence the v2.

[tboerger]

The wait, block and deploy features are important as they are actively used as far as I know.

[donny-dont]

The block was added by @dschmidt and has yet to ship in anything. My understanding from @bradrydzewski 's bug is that what is currently block is what he thinks wait should be.

[dschmidt]

The wait, block and deploy features are important as they are actively used as far as I know.

At least by me/ownCloud block is used :trollface:

The block was added by @dschmidt and has yet to ship in anything.

Agreed, probably not many people except for me are already using it, but I think it's an important feature and other people will follow once it's released.

My understanding from @bradrydzewski 's bug is that what is currently block is what he thinks wait should be.

Yeah, I tried to make the change non-breaking... which led to having two hard to distinguish options ... but I still like it better than reusing an existing option for a different function :-\

[donny-dont]

Its my understanding that wait was originally there because of a limitation of the API. Remember that drone-downstream is one of the first plugins way back in the pre-1.0 days. With the new build API its not needed so we can just use wait as wait for the triggered build to end.

[bradrydzewski]

the wait feature is only used because it was previously impossible to restart a build while it was running, which is why polling was implemented. This limitation no longer exists. Also we are moving from build restart to build create endpoint. So the wait command should no longer be relevant for this plugin.

[bradrydzewski]

if there are concerns about this being a breaking change ...

the wait field should be repurposed to wait for the build to complete and, if the build fails, return a non-zero exit code.

my recommendation would be that we deprecate and ignore wait going forward since it should no longer be needed. We can use block to ensure that it does not break existing configurations that define wait. Since wait is deprecated and ignored, I recommend we change from block_timeout to just timeout since there will only be a single timeout going forward.

[aknuds1]

Having a look at the feedback.

[aknuds1]

my recommendation would be that we deprecate and ignore wait going forward since it should no longer be needed. We can use block to ensure that it does not break existing configurations that define wait. Since wait is deprecated and ignored, I recommend we change from block_timeout to just timeout since there will only be a single timeout going forward.

@bradrydzewski For the sake of clarity, do you want for wait to be ignored or for it to enable block if set?

[aknuds1]

I've re-implemented deploy logic, but not sure what to do wrt. builds that are currently running. Please see my comment.

[dschmidt]

Is there any way forward with this?

[aknuds1]

I also hope we can progress on this :)

[andrewhowdencom]

Thanks for your work @aknuds1 ♥ I will be using this from now also.

[andrewhowdencom]

In testing this I found a condition in which if, for whatever reason, a new build is not created (such as if the build does not match a given set of triggers — in my case custom was missing from match events) — the plugin appeared to succeed, but then failed immediately thereafter when it was in the "block" step.

Notably, the returned build ID was 0 — like the request itself worked, but that while the event was successfully triggered, there were no builds generated.

Logs were:

2 | starting Drone build for __REPO__successfully started Drone build for __REPO__: #0
3 | with params:
4 | - DRONE_UPSTREAM_BUILD_NUMBER: 580
5 |  
6 | blocking until build is finished
7 | time="2021-02-17T20:50:53Z" level=error msg="execution failed: client error 404: {\"message\":\"sql: no rows in result set\"}\n"
8

This server is a little mutant though — theres a couple patches on there that aren't in main (yet) — but it might be worth digging into.

[leandrolerena]

Any news on this? There is a bug associated with this to my knowledge (parameter passing, old parameters get reused instead of new ones), which can lead to timeconsuming problems.

Is there a workaround for this? Will this PR be merged?