Wait for file log to drain before k6 exits #2414
Conversation
| ) (logrus.Hook, error) { | ||
| // TODO: fix this so it works correctly with relative paths from the CWD |
There was a problem hiding this comment.
Noticed that we don't explicitly check if we got a relative path here and instead just open exactly what we were given:
Line 108 in 1e28a3e
This works, for now, since we're directly using the os package and it will properly handle relative paths based on the current working directory. However, after we switch to afero.Fs here, we will need to explicitly account for it.
There was a problem hiding this comment.
can you expand this comment there with that info :)
There was a problem hiding this comment.
No need, I plan to fix it with a small commit in #2412 😅
There was a problem hiding this comment.
FWIW, this is the fix and the test to verify it: 7574bd2
| } | ||
|
|
||
| // FileHookFromConfigLine returns new fileHook hook. | ||
| func FileHookFromConfigLine( | ||
| ctx context.Context, fallbackLogger logrus.FieldLogger, line string, | ||
| ctx context.Context, fallbackLogger logrus.FieldLogger, line string, done chan struct{}, |
There was a problem hiding this comment.
This done channel looks more like the internal state. Do you think we should accept it as input?
Can a signature be like?
FileHookFromConfigLine(ctx context.Context, fallbackLogger logrus.FieldLogger, line string) (logrus.Hook, chan struct{}, error)Wdyt?
There was a problem hiding this comment.
Ideally, even with the doneCh <-chan sruct{}, but I guess it is outside of the scope of this PR
There was a problem hiding this comment.
hmm yeah, that is slightly better 👍
There was a problem hiding this comment.
actually, I think I'll refactor both this and LokiFromConfigLine in a separate PR, so I'll leave it as it is for now
| ) (logrus.Hook, error) { | ||
| // TODO: fix this so it works correctly with relative paths from the CWD |
There was a problem hiding this comment.
can you expand this comment there with that info :)
This closes #2413, a minor deficiency that slipped by in #2285
The integration test for this is unfortunately will have to wait until #2412 is merged. That is in fact how I found the issue, tried to write an integration for sending logs to a file and it failed because the file didn't contain the expected text at the end of the test 😅 So we'll have to merge this in v0.37.0 without a test 😞