kn: checksum bindings #1182
kn: checksum bindings #1182
Conversation
| if not opts.no_system_certs: | ||
| cmd.append(f'-v/etc/ssl:/etc/ssl') |
There was a problem hiding this comment.
Nit: f is unnecessary since we're not formatting any parameters in this string
| protected val serverPort: Int = ServerSocket(0).use { it.localPort } | ||
| protected val serverPort: Int = 54734 |
There was a problem hiding this comment.
Question: This will conflict if any other process (including a parallel build of this package in another workspace) is using port 54734. Is there a reason why it's necessary to use a static port?
There was a problem hiding this comment.
If we don't configure a port, it will default to 80, we could make it a randomly selected port if you think that will be safer?
There was a problem hiding this comment.
Random ports are marginally safer but still could conflict with an open port. I think we can still bind to an available address using Ktor's low-level KMP socket interfaces:
protected val serverPort: Int = runBlocking {
SelectorManager(coroutineContext).use { selector ->
aSocket(selector)
.tcp()
.bind()
.use { (it.localAddress as InetSocketAddress).port }
}
}There was a problem hiding this comment.
Nice, this works great
| attempt++ | ||
| try { | ||
| server.start() | ||
| println("test server listening on: $testHost:$serverPort") | ||
| break | ||
| } catch (cause: Throwable) { | ||
| if (attempt >= 10) throw cause | ||
| Thread.sleep(250L * attempt) | ||
| delay(250L * attempt) | ||
| } | ||
| } while (true) | ||
|
|
||
| ensureServerRunning() |
There was a problem hiding this comment.
Question: Isn't server.start() asynchronous? Won't removing ensureServerRunning() mean that it's possible to return from this method before the server is actually running?
There was a problem hiding this comment.
Good point, I refactored this to use ktor's monitoring API. I was having trouble with a KMP solution to ensureServerRunning by making a request.
| crc.update(input.encodeToByteArray(), 0, input.length) | ||
| assertEquals(2666930069U, crc.digestValue()) // checksum of "foobar" | ||
| assertEquals("nvYflQ==", crc.digest().encodeBase64String()) |
There was a problem hiding this comment.
Question: Why are we moving the digestValue checks from these tests?
There was a problem hiding this comment.
Our current JVM implementation of digestValue() does not reset the checksum and these tests were depending on that faulty behavior (calling digestValue() then digest() immediately after).
I've clarified the behavior in KDocs and also updated our JVM implementation. Let me know if you think we should keep the existing (arguably broken) behavior. KotlinNative / CRT will likely be unable to match that behavior though.
There was a problem hiding this comment.
Oh I see. If this was already broken behavior then you're probably right.
| def shell(command, cwd=None, check=True, capture_output=False): | ||
| """ | ||
| Run a command | ||
| :param command: command to run | ||
| :param cwd: the current working directory to change to before executing the command | ||
| :param check: flag indicating if the status code should be checked. When true an exception will be | ||
| thrown if the command exits with a non-zero exit status. | ||
| :returns: the subprocess CompletedProcess output | ||
| """ |
There was a problem hiding this comment.
Nit: Docs are missing for capture_output
3 participants
Issue #
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.