Merge ~cjwatson/turnip:cgit-timeout into turnip:master

Proposed by Colin Watson on 2019-07-05
Status: Merged
Approved by: Colin Watson on 2019-07-08
Approved revision: acb5fafdd667cbafb5536403ab1d509632169585
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/turnip:cgit-timeout
Merge into: turnip:master
Diff against target: 25 lines (+5/-1)
2 files modified
charm/turnip-pack-frontend-http/config.yaml (+4/-0)
charm/turnip-pack-frontend-http/templates/cgitwrap.j2 (+1/-1)
Reviewer Review Type Date Requested Status
Jonathan Hartley (community) 2019-07-05 Approve on 2019-07-08
Review via email: mp+369762@code.launchpad.net

Commit message

Time out cgit processes

We don't want these to hang around forever if something is going wrong.
cgit can legitimately take a while to render large files, so I started
out with a relatively lenient value of 30 seconds and we can see how
things look in practice.

To post a comment you must log in.
Jonathan Hartley (tartley) wrote :

For my education: This is the kind of change that isn't amenable to unit tests, but could be amenable to end-to-end tests, of a kind I think that we don't have any of, around this bit of code. Is that correct?

Are there reasons we don't have them that I ought to know about, other than the generic 'It's hard and might not be worth the effort" ?

I'm only asking for my education, no judgement! :-)

Thank you!

review: Approve
Colin Watson (cjwatson) wrote :

We indeed don't have end-to-end tests of this sort of thing at present. It's certainly doable in principle but as you say would be a very considerable amount of effort, as well as being slow to run because deploying the full stack with Juju takes quite a while. At the moment it isn't clear that it would repay the investment.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/charm/turnip-pack-frontend-http/config.yaml b/charm/turnip-pack-frontend-http/config.yaml
index 31f0476..56cac33 100644
--- a/charm/turnip-pack-frontend-http/config.yaml
+++ b/charm/turnip-pack-frontend-http/config.yaml
@@ -35,6 +35,10 @@ options:
35 type: string35 type: string
36 default: ''36 default: ''
37 description: Base64 encoded cgit session secret.37 description: Base64 encoded cgit session secret.
38 cgit_timeout:
39 type: int
40 default: 30
41 description: Time out cgit processes after this many seconds.
38 virtinfo_endpoint:42 virtinfo_endpoint:
39 type: string43 type: string
40 default: http://localhost:6543/githosting44 default: http://localhost:6543/githosting
diff --git a/charm/turnip-pack-frontend-http/templates/cgitwrap.j2 b/charm/turnip-pack-frontend-http/templates/cgitwrap.j2
index 7d1fcd2..0a0bdb9 100644
--- a/charm/turnip-pack-frontend-http/templates/cgitwrap.j2
+++ b/charm/turnip-pack-frontend-http/templates/cgitwrap.j2
@@ -1,5 +1,5 @@
1#! /bin/sh1#! /bin/sh
2set -e2set -e
33
4exec sudo -u {{ cgit_user }} /usr/lib/cgit/cgit.cgi "$@"4exec timeout --kill-after=15 {{ cgit_timeout }} sudo -u {{ cgit_user }} /usr/lib/cgit/cgit.cgi "$@"
55

Subscribers

People subscribed via source and target branches