Feature: Create builds directly instead of restarting old builds by aknuds1 · Pull Request #73 · drone-plugins/drone-downstream

aknuds1 · 2020-09-24T11:30:06Z

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

donny-dont · 2020-09-25T18:18:14Z

@aknuds1 can you look at targeting the v2 branch?

aknuds1 · 2020-09-25T18:56:27Z

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

aknuds1 · 2020-09-25T18:57:15Z

Will have to fix it up later.

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 · 2020-09-28T14:14:00Z

@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?

donny-dont

Marking up my nits. This particular plugin is widely used so I'm not approving it without consensus from @bradrydzewski and @tboerger

cmd/drone-downstream/main.go

donny-dont · 2020-09-28T15:22:15Z

plugin/impl.go

+					parts[0],
+				)
+			}
+


I don't see any benefit for moving the else if into this block.

I think it's far more understandable when reading and verifying the code, since it's a pain to get your head around in which exact cases the else block would originally fire. You also get the idiomatic Go error handling, with early return on an error and no corresponding else.

cmd/drone-downstream/config.go

donny-dont · 2020-09-28T15:26:49Z

go.mod

+	github.com/drone-plugins/drone-plugin-lib v0.4.0
+	github.com/drone/drone-go v1.4.0
 	github.com/joho/godotenv v1.3.0
+	github.com/stretchr/testify v1.4.0


@tboerger do you have any preferences on go testing modules? I don't have any strong opinions on it all.

No strong opinion on that, but isn't the standard library enough?

I can do without it, but it does make error checking in tests far less verbose.

donny-dont · 2020-09-28T15:35:38Z

cmd/drone-downstream/config.go

-		&cli.BoolFlag{
-			Name:        "wait",
-			Usage:       "Wait for any currently running builds to finish",
-			EnvVars:     []string{"PLUGIN_WAIT"},
-			Destination: &settings.Wait,
-		},
-		&cli.DurationFlag{
-			Name:        "timeout",
-			Value:       time.Duration(60) * time.Second,
-			Usage:       "How long to wait on any currently running builds",
-			EnvVars:     []string{"PLUGIN_WAIT_TIMEOUT"},
-			Destination: &settings.Timeout,
-		},


Believe @bradrydzewski 's initial ask was that wait and timeout would apply to the triggered build so should probably modify that accordingly.

OK, will try.

See @bradrydzewski ''s comments below at #73 (comment)

donny-dont · 2020-09-28T15:36:43Z

plugin/impl.go

-		branch string
-	)
-
+func parseRepoBranch(repo string) (string, string, string, error) {


I don't have any strong feelings on the modification to this function but maybe others do @tboerger ?

I think it's way better to return errors from this function at least, instead of leaving to the caller to surmise an error happened, without having the reason leading up to it.

aknuds1 · 2020-09-28T15:50:49Z

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

donny-dont · 2020-09-28T16:52:40Z

@aknuds1 with regards to the v2 branch we had this PR #70 which added a block option. Per #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 · 2020-09-28T17:23:17Z

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

donny-dont · 2020-09-28T17:26:01Z

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.

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

dschmidt · 2020-09-29T07:59:21Z

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

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 · 2020-09-29T18:19:10Z

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 · 2020-09-29T18:38:04Z

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 · 2020-09-29T18:46:15Z

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 · 2020-10-02T06:03:14Z

Having a look at the feedback.

aknuds1 · 2020-10-02T06:50:59Z

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?

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 · 2020-10-02T07:45:26Z

plugin/impl.go

-						return fmt.Errorf("unable to get last successful build for %s", entry)
-					}
-				}
+	fmt.Printf("deploying Drone build #%d of repo %s/%s\n", build.Number, owner, name)


@bradrydzewski Question: The old code, if the wait setting is enabled, will delay deploying/promoting a build until it has finished running. Considering you think the wait option is legacy, what should the new behaviour be?

aknuds1 · 2020-10-02T07:46:24Z

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

dschmidt · 2020-12-04T21:55:26Z

Is there any way forward with this?

aknuds1 · 2020-12-05T09:32:17Z

I also hope we can progress on this :)

andrewhowdencom · 2021-02-17T17:39:57Z

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

andrewhowdencom · 2021-02-17T20:54:19Z

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 · 2023-04-13T10:05:11Z

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?

aknuds1 changed the base branch from master to v2 September 25, 2020 18:56

aknuds1 added 2 commits September 28, 2020 15:47

Feature: Create builds directly instead of restarting old builds

59960ae

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Re-introduce blocking functionality

22807c8

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 force-pushed the chore/use-api branch from b702b1c to 22807c8 Compare September 28, 2020 14:12

donny-dont reviewed Sep 28, 2020

View reviewed changes

Remove unnecessary change

7ca17b1

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Reimplement deployment support

1fd96f8

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 commented Oct 2, 2020

View reviewed changes

aknuds1 requested review from donny-dont and tboerger October 2, 2020 07:45

Conversation

aknuds1 commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donny-dont commented Sep 25, 2020

Uh oh!

aknuds1 commented Sep 25, 2020

Uh oh!

aknuds1 commented Sep 25, 2020

Uh oh!

aknuds1 commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donny-dont left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aknuds1 Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aknuds1 commented Sep 28, 2020

Uh oh!

donny-dont commented Sep 28, 2020

Uh oh!

tboerger commented Sep 28, 2020

Uh oh!

donny-dont commented Sep 28, 2020

Uh oh!

dschmidt commented Sep 29, 2020

Uh oh!

donny-dont commented Sep 29, 2020

Uh oh!

bradrydzewski commented Sep 29, 2020

Uh oh!

bradrydzewski commented Sep 29, 2020

Uh oh!

aknuds1 commented Oct 2, 2020

Uh oh!

aknuds1 commented Oct 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aknuds1 commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dschmidt commented Dec 4, 2020

Uh oh!

aknuds1 commented Dec 5, 2020

Uh oh!

andrewhowdencom commented Feb 17, 2021

Uh oh!

andrewhowdencom commented Feb 17, 2021

Uh oh!

leandrolerena commented Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

aknuds1 commented Sep 24, 2020 •

edited

Loading

aknuds1 commented Sep 28, 2020 •

edited

Loading

aknuds1 Sep 29, 2020 •

edited

Loading

aknuds1 commented Oct 2, 2020 •

edited

Loading