Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix suppress_output_file bug where event_callback was not called #943

Merged
merged 1 commit into from Jan 4, 2022

Conversation

AlanCoding
Copy link
Member

Sorry, I had a major flaw with #937 and I should have tested that better before it was merged.

The handle was not passed to pexpect, and that was wrong. The handle is the way that it dispatches events to the event_callback. Even if you do not intend to write to the stdout file, this is necessary.

Design-wise, if suppress_output_file is given, we do want to use the stdout handle wrapper, but we do not want to use the stdout handle itself. I think this code structure articulates that about as best as possible. Alternatively, we could create a null handler and pass that to OutputEventFilter, instead of handling the None case.


Separately, I may have discovered a bug related to the test failures on the prior PR.

event_data['stdout'] = stdout_chunk[:-2] if len(stdout_chunk) > 2 else ""

This is assuming the .splitlines(True) leaves \r\n at the end of the line. This is... not universal. Because of that, the final character in the echo command gets truncated, because it has a \n without the \r. This looks completely fixable, but I wish to keep that separate.

@github-actions github-actions bot added the needs_triage New item that needs to be triaged label Dec 17, 2021
@AlanCoding AlanCoding marked this pull request as ready for review December 17, 2021 19:27
@AlanCoding AlanCoding requested a review from a team as a code owner December 17, 2021 19:27
@AlanCoding
Copy link
Member Author

Here's what the null file-like object looks like:

        class NullWriter:
            def write(self, s):
                pass

            def flush(self):
                pass

            def close(self):
                pass

This is not the implementation here, but it would replace StringIO in the alternative that I discussed. I'm torn as to which is the best.

@Shrews Shrews removed the needs_triage New item that needs to be triaged label Dec 21, 2021
@Shrews Shrews added the gate label Jan 4, 2022
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Shrews Shrews added gate and removed gate labels Jan 4, 2022
@ansible-zuul ansible-zuul bot merged commit 2d073fe into ansible:devel Jan 4, 2022
ansible-zuul bot pushed a commit that referenced this pull request Feb 1, 2022
Fix bug with truncating newline

This was followup promised in #943
Remaining concern
Of course I love .rstrip for the simplicity of it, but there may be a case where this method receives multiple empty lines, and it is expected to keep them. Like, it gets foo\n\n\n, and it is expected to write foo\n\n. This whole thing is frustrating and confusing. This scenario is probably unlikely to happen because verbose events only write 1 line at a time anyway. There might still be a case I'm missing that's important, and in that case, this could be adjusted so that only 1 trailing newline is removed from stdout_chunk, instead of all newline characters. I expect that would be less performant, so if this satisfies the current needs I'd keep it.

Reviewed-by: Alexander Sowitzki <dev@eqrx.net>
Reviewed-by: David Shrewsbury <None>
Reviewed-by: None <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants