feat(run): Add sigterm handler sample #1902

averikitsch · 2021-01-19T18:09:41Z

No description provided.

snippet-bot · 2021-01-19T18:09:47Z

Here is the summary of changes.

You are about to add 1 region tag.

run/sigterm-handler/main.go:18, tag cloudrun_sigterm_handler

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

run/sigterm-handler/main.go

run/sigterm-handler/main_test.go

averikitsch · 2021-03-16T19:05:15Z

The testing is not complete, but can you review if this addresses your comments? @ahmetb

ahmetb

Left some more comments.

run/sigterm-handler/main.go

run/sigterm-handler/Dockerfile

run/sigterm-handler/main.go

averikitsch · 2021-03-17T15:55:29Z

I need guidance on passing the "Build and lint" test. I ran goimports -w . && find . -name go.mod -execdir go mod tidy \; for the run/testing directory based on the error output, but the test isn't passing.

ahmetb · 2021-03-17T16:12:29Z

instead of creating a merge commit, maybe try git rebase origin/master on your last commit (that's not the merge commit).

ahmetb · 2021-03-18T22:22:55Z

run/sigterm-handler/main.go

+	defer func() {
+		// Add extra handling here to close any DB connections, redis, or flush logs
+		cancel() // Release resources
+	}()


you can shorten this by doing defer cancel() but this is also ok if Adam’s ok with it.

Normally I'd be for shortening this, but one of our main goals for the sample is to help developers know how to do these cleanup steps, so the extra flag for "drop code here" seems helpful.

I'd expect that cleanup code to happen outside this defer, ideally using the same ctx?

I might be missing something.

ahmetb · 2021-03-18T22:23:35Z

Please make sure to test it locally (I'm guessing by adding some time.Sleep in the handler function) and see if it behaves nicely.

averikitsch · 2021-03-19T17:15:16Z

Please make sure to test it locally (I'm guessing by adding some time.Sleep in the handler function) and see if it behaves nicely.

Works locally. Updating Kokoro to run e2e tests on presubmit #2005

run/sigterm-handler/main.go

tbpg · 2021-03-25T16:22:34Z

run/sigterm-handler/main.go

+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+
+	defer func() {
+		// Add extra handling here to close any DB connections, redis, or flush logs.


Not sure where the comment went. But, I'd expect any DB connections, etc. to handled outside this defer call. I might be missing something.

@grayside please advise

I'm not sure the idiomatic approach here, did a little searching to see how folks talk about this, and in the first 5 results, 2 of them do cleanup inside the defer and three just do them after the shutdown.

ahmetb · 2021-03-29T19:18:11Z

This was just asked at https://stackoverflow.com/a/66860299/54929.
It would be good to move this forward.

run/sigterm-handler/main.go

run/testing/sigterm_handler.e2e_test.go

internal/cloudrunci/cloudrunci.go

run/sigterm-handler/main.go

run/testing/sigterm_handler.e2e_test.go

averikitsch · 2021-03-31T17:53:32Z

Thank you @tbpg for your help. @grayside the code and tests are ready for a final review.

tbpg

One logging comment, otherwise looks good.

averikitsch · 2021-04-02T19:28:12Z

go1.16 tests are successful.
go1.11 tests have Pub/Sub sample failures

averikitsch · 2021-04-02T20:03:00Z

Admins please merge.

averikitsch requested review from ahmetb and grayside January 19, 2021 18:09

product-auto-label bot added the samples Issues that are directly related to samples. label Jan 19, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 19, 2021

averikitsch changed the title ~~Add sigterm handler sample~~ feat: Add sigterm handler sample Jan 19, 2021

averikitsch force-pushed the sigterm branch 2 times, most recently from d4143c4 to df2a722 Compare January 19, 2021 18:16

ahmetb reviewed Jan 19, 2021

View reviewed changes

run/sigterm-handler/main.go Outdated Show resolved Hide resolved

grayside requested changes Jan 19, 2021

View reviewed changes

run/sigterm-handler/main_test.go Show resolved Hide resolved

tbpg changed the title ~~feat: Add sigterm handler sample~~ feat(run): Add sigterm handler sample Jan 29, 2021

tbpg added the api: run Issues related to the Cloud Run API. label Jan 29, 2021

averikitsch marked this pull request as draft February 9, 2021 17:47

averikitsch force-pushed the sigterm branch from df2a722 to c476b83 Compare March 16, 2021 18:30

averikitsch force-pushed the sigterm branch from 2c18372 to 92067a8 Compare March 16, 2021 19:22

averikitsch marked this pull request as ready for review March 17, 2021 02:19

averikitsch requested a review from grayside March 17, 2021 02:19

averikitsch force-pushed the sigterm branch from 8217183 to 4bc5fbf Compare March 17, 2021 02:26

ahmetb suggested changes Mar 17, 2021

View reviewed changes

run/sigterm-handler/main.go Outdated Show resolved Hide resolved

run/sigterm-handler/Dockerfile Show resolved Hide resolved

run/sigterm-handler/main.go Show resolved Hide resolved

averikitsch force-pushed the sigterm branch from bd6e399 to 1fab609 Compare March 18, 2021 15:09

averikitsch requested a review from ahmetb March 18, 2021 15:38

ahmetb reviewed Mar 18, 2021

View reviewed changes

ahmetb approved these changes Mar 18, 2021

View reviewed changes

averikitsch added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 19, 2021

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 19, 2021

tbpg reviewed Mar 25, 2021

View reviewed changes

grayside reviewed Mar 29, 2021

View reviewed changes

run/sigterm-handler/main.go Outdated Show resolved Hide resolved

averikitsch added 2 commits March 29, 2021 13:38

Don't fatal on server error

9043cca

Merge branch 'master' into sigterm

d301f72

ahmetb reviewed Mar 29, 2021

View reviewed changes

run/testing/sigterm_handler.e2e_test.go Outdated Show resolved Hide resolved

Update to idiomatic go

e86c015

averikitsch force-pushed the sigterm branch from 4ee91ce to e86c015 Compare March 30, 2021 17:44

Update testing

b4a535d

tbpg reviewed Mar 31, 2021

View reviewed changes

averikitsch added 2 commits March 31, 2021 09:08

Respond to comments

dae3a0f

lint

79c29ee

averikitsch force-pushed the sigterm branch from 76969b6 to 79c29ee Compare March 31, 2021 16:22

averikitsch added 3 commits March 31, 2021 10:53

Merge branch 'master' into sigterm

4d11b54

Merge branch 'master' into sigterm

52db080

lint

f1c6a3d

tbpg approved these changes Apr 1, 2021

View reviewed changes

grayside approved these changes Apr 2, 2021

View reviewed changes

tbpg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 5, 2021

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 5, 2021

tbpg merged commit e623c04 into master Apr 5, 2021

tbpg deleted the sigterm branch April 5, 2021 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(run): Add sigterm handler sample #1902

feat(run): Add sigterm handler sample #1902

averikitsch commented Jan 19, 2021

snippet-bot bot commented Jan 19, 2021 •

edited

Loading

averikitsch commented Mar 16, 2021

ahmetb left a comment

averikitsch commented Mar 17, 2021

ahmetb commented Mar 17, 2021

ahmetb Mar 18, 2021

grayside Mar 23, 2021

tbpg Mar 24, 2021

ahmetb commented Mar 18, 2021 •

edited

Loading

averikitsch commented Mar 19, 2021

tbpg Mar 25, 2021

averikitsch Mar 25, 2021

grayside Mar 29, 2021

ahmetb commented Mar 29, 2021

averikitsch commented Mar 31, 2021

tbpg left a comment

averikitsch commented Apr 2, 2021

averikitsch commented Apr 2, 2021

feat(run): Add sigterm handler sample #1902

feat(run): Add sigterm handler sample #1902

Conversation

averikitsch commented Jan 19, 2021

snippet-bot bot commented Jan 19, 2021 • edited Loading

averikitsch commented Mar 16, 2021

ahmetb left a comment

Choose a reason for hiding this comment

averikitsch commented Mar 17, 2021

ahmetb commented Mar 17, 2021

ahmetb Mar 18, 2021

Choose a reason for hiding this comment

grayside Mar 23, 2021

Choose a reason for hiding this comment

tbpg Mar 24, 2021

Choose a reason for hiding this comment

ahmetb commented Mar 18, 2021 • edited Loading

averikitsch commented Mar 19, 2021

tbpg Mar 25, 2021

Choose a reason for hiding this comment

averikitsch Mar 25, 2021

Choose a reason for hiding this comment

grayside Mar 29, 2021

Choose a reason for hiding this comment

ahmetb commented Mar 29, 2021

averikitsch commented Mar 31, 2021

tbpg left a comment

Choose a reason for hiding this comment

averikitsch commented Apr 2, 2021

averikitsch commented Apr 2, 2021

snippet-bot bot commented Jan 19, 2021 •

edited

Loading

ahmetb commented Mar 18, 2021 •

edited

Loading