cmd/image-builder: add deployment channel as field to the logs #1299
Conversation
|
keeping as draft until I understand why the (unrelated) tests fail on my PC |
schuellerf
force-pushed
the
add-deployment-channel-to-logs
branch
from
9ae47b1
to
a485cb6
Compare
schuellerf
force-pushed
the
add-deployment-channel-to-logs
branch
from
a485cb6
to
9c9ae7d
Compare
schuellerf
force-pushed
the
add-deployment-channel-to-logs
branch
3 times, most recently
from
557a605
to
1460687
Compare
|
/retest |
1 similar comment
|
/retest |
this is critical for tests where e.g. the bind to the port fails. When ignored (as it was) the test will connect to the application running on that port, resulting in strange behavior
schuellerf
force-pushed
the
add-deployment-channel-to-logs
branch
from
1460687
to
542097a
Compare
| @@ -191,7 +191,10 @@ func startServer(t *testing.T, tscc *testServerClientsConf, conf *ServerConfig) | |||
| require.NoError(t, err) | |||
| // execute in parallel b/c .Run() will block execution | |||
| go func() { | |||
| _ = echoServer.Start("localhost:8086") | |||
| err = echoServer.Start("localhost:8086") | |||
| if err.Error() != "http: Server closed" { | |||
There was a problem hiding this comment.
(niptick) I personally would prefer if this commit could be it's own PR (alternatively the PR title could get updated maybe?). When going over the project history it's often nice to see the what PR a commit belonged to and here the PR title/intention and this particular fix feel a bit disconnected.
About the change itself:
It's probably best to compare error types instead of strings here, i.e. if err != http.ErrServerClosed
How does this work exactly? Will we always get ErrServerClosed here or is it random (either nil or ErrServerClosed?).
Bonus points for: the comment above talks about .Run() but the code uses .Start() so we could clean this up too. And while add it we could use your nice GetFreePort() helper from osbuild/osbuild-composer@55c5602 :)
There was a problem hiding this comment.
ServerClose always happens on regular shutdown...
I already did implement with GetFreePort() (locally) but stopped after changing ~1000 strings to take a dynamic host&port 😑
Will do this "major rework" at some other point in time
There was a problem hiding this comment.
I did a quick sed experiement and opened #1303 which may help, I did not really check much, it was mostly sed doing it's thing but it might be useful. No getFreePort there but that should be easy to hook up once the other pieces are there.
It also include the tiny http.ErrServerClosed tweak in #1304
| - name: Run unit tests | ||
| run: go test -v -race -covermode=atomic -coverprofile=coverage.txt -coverpkg=./... ./... | ||
| run: make unit-tests |
There was a problem hiding this comment.
The makefile has a comment re-implementing .github/workflows/tests.yml as close as possible which probably needs updating now :)
There was a problem hiding this comment.
Thanks - good catch 🙈
add deployment channel as field to the logs similar to
osbuild/osbuild-composer#4285
and based on
osbuild/osbuild-composer#4301