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

Expose the output_type option when streaming, and allow both stdout/stderr to be captured #209

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

tjgalvin
Copy link
Contributor

This is an untested PR, but one I hope is fairly straightforward. I have a container whose program within it only outputs on the stderr. When streaming I am via the Client.execute method I am unable to capture these messages and log them.

This PR aims to:

  • expose the output_type option in execute (I have called it stream_type for clarity in the call into execute)
  • ability to stream both the stdout and stderr.
    ?
    Would someone be able to give this a look over and test?

Copy link
Member

@vsoch vsoch left a comment

Choose a reason for hiding this comment

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

A small change is OK! Could you please ensure that:

  • you've run black and linting
  • bump the version in version.py
  • add a note to the CHANGELOG with the corresponding version

Thank you!

@@ -122,12 +122,14 @@ def stream_command(
sudo_options: string or list of strings that will be passed as options to sudo

"""
if output_type not in ["stdout", "stderr"]:
if output_type not in ["stdout", "stderr", "both"]:
bot.exit("Invalid output type %s. Must be stderr or stdout." % output_type)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bot.exit("Invalid output type %s. Must be stderr or stdout." % output_type)
bot.exit("Invalid output type %s. Must be stderr, stdout, or both" % output_type)

@vsoch vsoch changed the title Exose the output_type option when streaming, and allow both stdout/stderr to be captured Expose the output_type option when streaming, and allow both stdout/stderr to be captured Oct 25, 2023
@tjgalvin
Copy link
Contributor Author

Alright, I believe I have run black on the two changes, bumped the version and added to the change log.

Please do let me know if there is anything else that comes to mind. This was a very quick read for me, and I have not explicitly tested this new code path. It is small enough where I think it is OK.

@vsoch
Copy link
Member

vsoch commented Oct 25, 2023

I have not explicitly tested this new code path

if you are doing this for your own use and not adding a test here, I do need to ask you to at least test your local case (and show the result here).

For black, I recommend we pin to 23.3.0 (before the big change that produced the error and re-run).

@tjgalvin
Copy link
Contributor Author

tjgalvin commented Oct 25, 2023

I have added a unit test to the test suite that attempts to verify this (and future proof things).

@tjgalvin
Copy link
Contributor Author

And after patching my fork into my environment and testing it seems to be working as expected. I now have logs where there were no logs before.

@vsoch vsoch merged commit 3a885ab into singularityhub:master Oct 25, 2023
4 checks passed
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.

2 participants