Fix erroneous carriage return character (\r) in the output of remote shell script and command actions #4623
Conversation
stdout and stderr data with \n when pty is used. When pty is used, default behavior is to replace all new line characters \n with \r\n which is almost never desired.
|
NOTE: We already have end to end test for it in st2tests repo, which wasn't failing due to the bug in our assertion code in st2tests repo which I fixed here - StackStorm/st2tests#154. |
| output = sanitize_output(input_str, uses_pty=False) | ||
| self.assertEqual(expected_output, output) | ||
|
|
||
| # 2. pty is used, \r\n should be replaced with \n |
There was a problem hiding this comment.
Should this be split out into its own test function?
There was a problem hiding this comment.
I decided to split it into two methods - 3524e05.
When to split the tests and when not to is not always totally clear - when a test needs to be isolated and potentially also requires on setUp and tearDown method to run then it's clear that such tests need to be separate methods, but in scenarios like that it's less clear / needed.
In general, I do agree with you though - I prefer separate methods since it provides more user-friendly test runner output (and unless setUp / tearDown add a huge overhead then there are really no drawbacks).
…into fix_carriage_return_char
…entry Upgrade notes entry for change in StackStorm/st2/pull/4623
Ladies and gentlemen, long time mystery of "erroneous" carriage return characters sometimes found in stdout and stderr of remote runner actions has finally been solved.
While working on and fixing a totally unrelated issue in StackStorm/st2tests#154, I found our that sometimes
\rcharacter ends up in action stdout even though the underlying script only prints\nand not\r\n.It turns out when pty is used, default terminal option is
onlcrwhich translates\ninto\r\n(see https://stackoverflow.com/questions/7812142/how-to-toggle-cr-lf-in-gnu-screen and https://linux.die.net/man/1/stty for details).This is not desired in our remote actions output (in fact, we had a lot of users report such issues in Slack and similar).
It looks like we already tried to resolve that all the way back in 2015 (eca3751), but our solution / workaround for it wasn't complete. We only removed those characters from the end of the string, but we didn't do anything with
\r\nin the middle of the string.TODO