Fixing reloading when rotate-logs is true

[ktak-007]

Fixing of issue #296 "Reloading is broken in v2.1 with rotate-logs: True"

[jezen]

Awesome! Well done 👏

I would like to evidence that this is well and truly fixed. I think the way to do this is probably to use the integrated tests that I added recently, unless of course there's a nicer way to test this with only Hspec. What do you think?

[ktak-007]

I would like to evidence that this is well and truly fixed. I think the way to do this is probably to use the integrated tests that I added recently, unless of course there's a nicer way to test this with only Hspec. What do you think?

Both ideas are great, but, I think, to make integration test we have to create a builder for "foo" application, build it during test, put it to "incoming/" dir, test the application, recompile it with some changed parameter (to get different output), put it again and test the application again - it must get a new output.

For hspec test we have to check withLogger function but it is not exported from Keter.App module.

[jezen]

I don't think the integrated test would need a compilation step. I think this could be a Python or Bash script, for example.

Maybe like this

from http.server import HTTPServer, BaseHTTPRequestHandler

class SimpleHandler(BaseHTTPRequestHandler):
    def do_GET(self):
        self.send_response(200)
        self.send_header("Content-type", "text/plain")
        self.end_headers()
        self.wfile.write(b"Hello, world!")

HTTPServer(("0.0.0.0", 8000), SimpleHandler).serve_forever()

Or even just like this, with netcat

while true; do echo -e "HTTP/1.1 200 OK\n\nHello, world!" | nc -l -p 8000; done

If this can be done in a sensible way with Hspec, that would probably be better. Is there any reason not to export withLogger so that it can be tested?

[ulidtko]

@jezen is right, webapp bundle doesn't need a "builder", it can be super minimal.

I'd been using this 2-file thing, whole bundle ready-to-go <50 lines:

#!/usr/bin/env python3
import http.server
import os,sys
import socketserver
from http import HTTPStatus
from time import sleep

class Handler(http.server.SimpleHTTPRequestHandler):
    def do_GET(self):
        self.send_response(HTTPStatus.OK)
        self.end_headers()
        body = f"Hello from PID {os.getpid()} listening on port {listenport} !\n\nThis is v4\n"
        self.wfile.write(body.encode('ascii'))

listenport = int(os.getenv('PORT', 8000))
for i in range(10):
    print(f"Slow service starting up... {i}/10")
    sleep(1.0)
print(f"Listening on port {listenport}")
httpd = socketserver.TCPServer(('', listenport), Handler)
httpd.serve_forever()

#-- variation: webapp that's broken and fails to start
#httpd.serve_forever()
#print("Actually I'm a broken webapp, cannot start!")
#sys.exit(1)

plus the obvious config/keter.yml

stanzas:
- type: webapp
  hosts:
  - localhost
  ssl: false
  env:
    PYTHONUNBUFFERED: '1'
  exec: ../app/web-hello.py

Note:

1. This dummy.keter simulates slow start. This is realistic for test, as real bundles do take non-trivial time to fully start.
2. There's a mutation with sys.exit(1) — that one simulates a broken/crashing bundle. Also realistic, and IMO essential in keter's integration tests.

[ulidtko]

Testing is a deep topic; let's not detract the PR there. @jezen I don't have a Nix setup, so IMO it'd be best for you to collaborate with @ktak-007 on your vision of integration tests.

@ktak-007 I tested your patch by hand; seems to work, giving LGTM.

The only thing I'll ask is adherence to https://www.conventionalcommits.org — your commit title line is way overboard in terms of length. Cut it down. ~52 chars.

[ulidtko]

Commit title:

Fixing of issue https://github.com/snoyberg/keter/issues/296 "Reloading is broken in v2.1 with rotate-logs: True"

Don't put URLs in git commit titles. You can mention issues by tags, #296.

Suggested wording:

Fix reloading with rotate-logs: True

Resolves #296

Use git commit --amend to fix the title, and force-push to your branch, the PR will auto-update.

[ktak-007]

I did it.

[jappeace]

yeah I'd just merge it as is if it works

we haven't had a test suite in forever so now we do have we can figure out how to use it as a separate issue

[ulidtko]

Well no @jappeace, there's a ~100 lines Tasty test-suite... with what, 3 tests in it 😑

Regardless, I'm OK with merging this with only a test by hand. Whichever way, engineering the scenario into auto-tests will be another piece of work.

[jezen]

I'll have a go at writing the integrated test for this. If I don't make any progress, I'll just merge this as is in a few hours.

Update: I've made good progress here, so I'll work on this a little more.

[jezen]

Ok, I've written the integrated test. I'm satisfied that it faithfully reproduces this specific failure, and applying @ktak-007's change on top of it makes the test pass.

I'll merge this, and then I'll merge the test.