From 3a885ab11e38c5fbe76413c2ee4b1c26fb2925f0 Mon Sep 17 00:00:00 2001 From: tjgalvin Date: Thu, 26 Oct 2023 01:49:53 +0800 Subject: [PATCH] Expose the output_type option when streaming, and allow both stdout/stderr to be captured (#209) * allow both output stdout stderr to be emitted. Expose option to allow capture * updated changelog, version bump, error message * added a basic stream test * linting and pin black to 23.3.0 Signed-off-by: vsoch Co-authored-by: tgalvin Co-authored-by: Tim Galvin --- .github/dev-requirements.txt | 2 +- CHANGELOG.md | 1 + setup.py | 1 - spython/client/__init__.py | 2 -- spython/client/recipe.py | 2 -- spython/instance/__init__.py | 1 - spython/instance/cmd/start.py | 1 - spython/main/base/command.py | 2 -- spython/main/base/generate.py | 1 - spython/main/build.py | 1 - spython/main/execute.py | 7 +++++-- spython/main/export.py | 1 - spython/main/instances.py | 2 -- spython/main/parse/parsers/base.py | 2 -- spython/main/parse/parsers/docker.py | 6 ------ spython/main/parse/parsers/singularity.py | 6 ------ spython/main/parse/recipe.py | 1 - spython/main/parse/writers/docker.py | 2 -- spython/main/parse/writers/singularity.py | 4 ---- spython/main/pull.py | 2 -- spython/oci/__init__.py | 1 - spython/oci/cmd/actions.py | 3 --- spython/oci/cmd/states.py | 5 ----- spython/tests/test_client.py | 19 +++++++++++++++++++ spython/tests/test_conversion.py | 1 - spython/utils/terminal.py | 11 +++++++---- spython/version.py | 2 +- 27 files changed, 34 insertions(+), 55 deletions(-) diff --git a/.github/dev-requirements.txt b/.github/dev-requirements.txt index 0b29a88..6116de3 100644 --- a/.github/dev-requirements.txt +++ b/.github/dev-requirements.txt @@ -1,4 +1,4 @@ pre-commit -black +black==23.3.0 isort flake8 diff --git a/CHANGELOG.md b/CHANGELOG.md index 25e3685..1c53755 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ The client here will eventually be released as "spython" (and eventually to singularity on pypi), and the versions here will coincide with these releases. ## [master](https://github.com/singularityhub/singularity-cli/tree/master) + - exposed the stream type option, and ability to capture both stdout and stderr when stream=True (0.3.1) - dropping support for Singularity 2.x (0.3.0) - add comment out of STOPSIGNAL (0.2.14) - sudo `-E` flag should not be provided by default (0.2.13) diff --git a/setup.py b/setup.py index 72756f2..6482d7c 100644 --- a/setup.py +++ b/setup.py @@ -75,7 +75,6 @@ def get_requirements(lookup=None): if __name__ == "__main__": - INSTALL_REQUIRES = get_requirements(lookup) TESTS_REQUIRES = get_requirements(lookup) diff --git a/spython/client/__init__.py b/spython/client/__init__.py index 1cd31a3..26e3c88 100644 --- a/spython/client/__init__.py +++ b/spython/client/__init__.py @@ -13,7 +13,6 @@ def get_parser(): - parser = argparse.ArgumentParser( description="Singularity Client", formatter_class=argparse.RawTextHelpFormatter, @@ -147,7 +146,6 @@ def version(): def main(): - parser = get_parser() def print_help(return_code=0): diff --git a/spython/client/recipe.py b/spython/client/recipe.py index 57be3cb..7f63c96 100644 --- a/spython/client/recipe.py +++ b/spython/client/recipe.py @@ -74,7 +74,6 @@ def main(args, options, parser): force = True if args.json: - if outfile is not None: if not os.path.exists(outfile): if force: @@ -85,7 +84,6 @@ def main(args, options, parser): print(json.dumps(recipeParser.recipe.json(), indent=4)) else: - # Do the conversion recipeWriter = writer(recipeParser.recipe) result = recipeWriter.convert(runscript=entrypoint, force=force) diff --git a/spython/instance/__init__.py b/spython/instance/__init__.py index 2910243..275cf34 100644 --- a/spython/instance/__init__.py +++ b/spython/instance/__init__.py @@ -93,7 +93,6 @@ def _update_metadata(self, kwargs=None): # Add acceptable arguments for arg in ["pid", "name", "ip_address", "log_err_path", "log_out_path", "img"]: - # Skip over non-iterables: if arg in kwargs: setattr(self, arg, kwargs[arg]) diff --git a/spython/instance/cmd/start.py b/spython/instance/cmd/start.py index 8a11203..da64346 100644 --- a/spython/instance/cmd/start.py +++ b/spython/instance/cmd/start.py @@ -49,7 +49,6 @@ def start( # If an image isn't provided, we have an initialized instance if image is None: - # Not having this means it was called as a command, without an image if not hasattr(self, "_image"): bot.exit("Please provide an image, or create an Instance first.") diff --git a/spython/main/base/command.py b/spython/main/base/command.py index 17a626f..40fe93e 100644 --- a/spython/main/base/command.py +++ b/spython/main/base/command.py @@ -66,7 +66,6 @@ def generate_bind_list(self, bindlist=None): bindlist = bindlist.split(" ") for bind in bindlist: - # Still cannot be None if bind: bot.debug("Adding bind %s" % bind) @@ -113,7 +112,6 @@ def run_command( environ=None, background=False, ): - """ Run_command is a wrapper for the global run_command, checking first for sudo and exiting on error if needed. The message is returned as diff --git a/spython/main/base/generate.py b/spython/main/base/generate.py index 532aaf0..f3ece0a 100644 --- a/spython/main/base/generate.py +++ b/spython/main/base/generate.py @@ -11,7 +11,6 @@ class RobotNamer: - _descriptors = [ "chunky", "buttery", diff --git a/spython/main/build.py b/spython/main/build.py index 360dbf7..2753ec8 100644 --- a/spython/main/build.py +++ b/spython/main/build.py @@ -31,7 +31,6 @@ def build( sudo_options=None, singularity_options=None, ): - """build a singularity image, optionally for an isolated build (requires sudo). If you specify to stream, expect the image name and an iterator to be returned. diff --git a/spython/main/execute.py b/spython/main/execute.py index ed75553..fd3654d 100644 --- a/spython/main/execute.py +++ b/spython/main/execute.py @@ -29,6 +29,7 @@ def execute( sudo_options=None, quiet=True, environ=None, + stream_type="stdout", ): """execute: send a command to a container @@ -51,6 +52,7 @@ def execute( and message result not (default) quiet: Do not print verbose output. environ: extra environment to add. + stream_type: Sets which output stream from the singularity command should be return. Values are 'stdout', 'stderr', 'both'. """ from spython.utils import check_install @@ -68,7 +70,6 @@ def execute( image = None if command is not None: - # No image provided, default to use the client's loaded image if image is None: image = self._get_uri() @@ -115,7 +116,9 @@ def execute( quiet=quiet, environ=environ, ) - return stream_command(cmd, sudo=sudo, sudo_options=sudo_options) + return stream_command( + cmd, sudo=sudo, sudo_options=sudo_options, output_type=stream_type + ) bot.exit("Please include a command (list) to execute.") diff --git a/spython/main/export.py b/spython/main/export.py index 8d3d8eb..e912a67 100644 --- a/spython/main/export.py +++ b/spython/main/export.py @@ -19,7 +19,6 @@ def export( sudo=False, singularity_options=None, ): - """export will export an image, sudo must be used. Since we have Singularity versions after 3, export is replaced with building into a sandbox. diff --git a/spython/main/instances.py b/spython/main/instances.py index f75b9a4..573cb56 100644 --- a/spython/main/instances.py +++ b/spython/main/instances.py @@ -63,7 +63,6 @@ def list_instances( # Success, we have instances if output["return_code"] == 0: - instances = json.loads(output["message"][0]).get("instances", {}) # Does the user want instance objects instead? @@ -71,7 +70,6 @@ def list_instances( if not return_json: for i in instances: - # If the user has provided a name, only add instance matches if name is not None: if name != i["instance"]: diff --git a/spython/main/parse/parsers/base.py b/spython/main/parse/parsers/base.py index 0661394..42bd23f 100644 --- a/spython/main/parse/parsers/base.py +++ b/spython/main/parse/parsers/base.py @@ -48,7 +48,6 @@ def __init__(self, filename, load=True): self.recipe = {"spython-base": Recipe(self.filename)} if self.filename: - # Read in the raw lines of the file self.lines = read_file(self.filename) @@ -69,7 +68,6 @@ def _run_checks(self): attempting parsing. """ if self.filename is not None: - # Does the recipe provided exist? if not os.path.exists(self.filename): bot.exit("Cannot find %s, is the path correct?" % self.filename) diff --git a/spython/main/parse/parsers/docker.py b/spython/main/parse/parsers/docker.py index ead7df5..ef8d356 100644 --- a/spython/main/parse/parsers/docker.py +++ b/spython/main/parse/parsers/docker.py @@ -14,7 +14,6 @@ class DockerParser(ParserBase): - name = "docker" def __init__(self, filename="Dockerfile", load=True): @@ -49,7 +48,6 @@ def parse(self): previous = None for line in self.lines: - parser = self._get_mapping(line, parser, previous) # Parse it, if appropriate @@ -147,7 +145,6 @@ def _arg(self, line): # Try to extract arguments from the line for arg in line: - # An undefined arg cannot be used if "=" not in arg: bot.warning( @@ -197,7 +194,6 @@ def parse_env(self, envlist): exports = [] for env in envlist: - pieces = re.split("( |\\\".*?\\\"|'.*?')", env) pieces = [p for p in pieces if p.strip()] @@ -205,7 +201,6 @@ def parse_env(self, envlist): current = pieces.pop(0) if current.endswith("="): - # Case 1: ['A='] --> A= nextone = "" @@ -243,7 +238,6 @@ def _copy(self, lines): lines = self._setup("COPY", lines) for line in lines: - # Take into account multistage builds layer = None if line.startswith("--from"): diff --git a/spython/main/parse/parsers/singularity.py b/spython/main/parse/parsers/singularity.py index a89f8c3..70214fe 100644 --- a/spython/main/parse/parsers/singularity.py +++ b/spython/main/parse/parsers/singularity.py @@ -13,7 +13,6 @@ class SingularityParser(ParserBase): - name = "singularity" def __init__(self, filename="Singularity", load=True): @@ -51,7 +50,6 @@ def _setup(self, lines): bot.warning("SETUP is error prone, please check output.") for line in lines: - # For all lines, replace rootfs with actual root / line = re.sub("[$]{?SINGULARITY_ROOTFS}?", "", "$SINGULARITY_ROOTFS") @@ -171,7 +169,6 @@ def _run(self, lines): # Multiple line runscript needs multiple lines written to script if len(lines) > 1: - bot.warning("More than one line detected for runscript!") bot.warning("These will be echoed into a single script to call.") self._write_script("/entrypoint.sh", lines) @@ -257,7 +254,6 @@ def _load_section(self, lines, section, layer=None): members = [] while True: - if not lines: break next_line = lines[0] @@ -278,7 +274,6 @@ def _load_section(self, lines, section, layer=None): # Add the list to the config if members and section is not None: - # Get the correct parsing function parser = self._get_mapping(section) @@ -309,7 +304,6 @@ def load_recipe(self): comments = [] while lines: - # Clean up white trailing/leading space line = lines.pop(0) stripped = line.strip() diff --git a/spython/main/parse/recipe.py b/spython/main/parse/recipe.py index 27b48cc..2ae4859 100644 --- a/spython/main/parse/recipe.py +++ b/spython/main/parse/recipe.py @@ -23,7 +23,6 @@ class Recipe: """ def __init__(self, recipe=None, layer=1): - self.cmd = None self.comments = [] self.entrypoint = None diff --git a/spython/main/parse/writers/docker.py b/spython/main/parse/writers/docker.py index 92f80c5..50b59bd 100644 --- a/spython/main/parse/writers/docker.py +++ b/spython/main/parse/writers/docker.py @@ -45,7 +45,6 @@ class DockerWriter(WriterBase): - name = "docker" def __init__(self, recipe=None): # pylint: disable=useless-super-delegation @@ -161,7 +160,6 @@ def write_lines(label, lines): result = [] continued = False for line in lines: - # Skip comments and empty lines if line.strip() == "" or line.strip().startswith("#"): continue diff --git a/spython/main/parse/writers/singularity.py b/spython/main/parse/writers/singularity.py index 907e72a..7ae233b 100644 --- a/spython/main/parse/writers/singularity.py +++ b/spython/main/parse/writers/singularity.py @@ -13,7 +13,6 @@ class SingularityWriter(WriterBase): - name = "singularity" def __init__(self, recipe=None): # pylint: disable=useless-super-delegation @@ -52,7 +51,6 @@ def convert(self, runscript="/bin/bash", force=False): # Write each layer to new file for stage, parser in self.recipe.items(): - # Set the first and active stage self.stage = stage @@ -111,9 +109,7 @@ def _create_runscript(self, default="/bin/bash", force=False): # Only look at Docker if not enforcing default if not force: - if self.recipe[self.stage].entrypoint is not None: - # The provided entrypoint can be a string or a list if isinstance(self.recipe[self.stage].entrypoint, list): entrypoint = " ".join(self.recipe[self.stage].entrypoint) diff --git a/spython/main/pull.py b/spython/main/pull.py index c4eee6f..6ff3e69 100644 --- a/spython/main/pull.py +++ b/spython/main/pull.py @@ -24,7 +24,6 @@ def pull( quiet=False, singularity_options=None, ): - """pull will pull a singularity hub or Docker image Parameters @@ -92,7 +91,6 @@ def pull( # Option 3: A custom name we can predict (not commit/hash) and can also show else: - # As of Singularity 3.x (at least 3.8) output goes to stderr return final_image, stream_command(cmd, sudo=False, output_type="stderr") diff --git a/spython/oci/__init__.py b/spython/oci/__init__.py index 6cf061c..64a0a78 100644 --- a/spython/oci/__init__.py +++ b/spython/oci/__init__.py @@ -9,7 +9,6 @@ class OciImage(ImageBase): - # Default functions of client don't use sudo sudo = False diff --git a/spython/oci/cmd/actions.py b/spython/oci/cmd/actions.py index 80758ab..6c151f9 100644 --- a/spython/oci/cmd/actions.py +++ b/spython/oci/cmd/actions.py @@ -19,7 +19,6 @@ def run( singularity_options=None, log_format="kubernetes", ): - """run is a wrapper to create, start, attach, and delete a container. Equivalent command line example: @@ -57,7 +56,6 @@ def create( log_format="kubernetes", singularity_options=None, ): - """use the client to create a container from a bundle directory. The bundle directory should have a config.json. You must be the root user to create a runtime. @@ -104,7 +102,6 @@ def _run( log_format="kubernetes", singularity_options=None, ): - """_run is the base function for run and create, the only difference between the two being that run does not have an option for sync_socket. diff --git a/spython/oci/cmd/states.py b/spython/oci/cmd/states.py index 1b2a521..163007c 100644 --- a/spython/oci/cmd/states.py +++ b/spython/oci/cmd/states.py @@ -11,7 +11,6 @@ def state( self, container_id=None, sudo=None, sync_socket=None, singularity_options=None ): - """get the state of an OciImage, if it exists. The optional states that can be returned are created, running, stopped or (not existing). @@ -47,7 +46,6 @@ def state( result = self._run_command(cmd, sudo=sudo, quiet=True) if result is not None: - # If successful, a string is returned to parse if isinstance(result, str): return json.loads(result) @@ -56,7 +54,6 @@ def state( def _state_command( self, container_id=None, command="start", sudo=None, singularity_options=None ): - """A generic state command to wrap pause, resume, kill, etc., where the only difference is the command. This function will be unwrapped if the child functions get more complicated (with additional arguments). @@ -90,7 +87,6 @@ def _state_command( def start(self, container_id=None, sudo=None, singularity_options=None): - """start a previously invoked OciImage, if it exists. Equivalent command line example: @@ -112,7 +108,6 @@ def start(self, container_id=None, sudo=None, singularity_options=None): def kill(self, container_id=None, sudo=None, signal=None, singularity_options=None): - """stop (kill) a started OciImage container, if it exists Equivalent command line example: diff --git a/spython/tests/test_client.py b/spython/tests/test_client.py index 3531114..0703a8c 100644 --- a/spython/tests/test_client.py +++ b/spython/tests/test_client.py @@ -65,6 +65,25 @@ def test_execute_with_return_code(docker_container): assert result["return_code"] == 0 +def test_execute_with_stream(docker_container): + output = Client.execute(docker_container[1], "ls /", stream=True) + message = "".join(list(output)) + assert "tmp\nusr\nvar" in message + + output = Client.execute( + docker_container[1], "ls /", stream=True, stream_type="both" + ) + message = "".join(list(output)) + assert "tmp\nusr\nvar" in message + + # ls / should be successful, so there will be no stderr + output = Client.execute( + docker_container[1], "ls /", stream=True, stream_type="stderr" + ) + message = "".join(list(output)) + assert "tmp\nusr\nvar" not in message + + @pytest.mark.parametrize("return_code", [True, False]) def test_execute_with_called_process_error( capsys, docker_container, return_code, tmp_path diff --git a/spython/tests/test_conversion.py b/spython/tests/test_conversion.py index d4c053d..37b25f6 100644 --- a/spython/tests/test_conversion.py +++ b/spython/tests/test_conversion.py @@ -48,7 +48,6 @@ def test_docker2singularity(test_data, tmp_path): def test_singularity2docker(test_data, tmp_path): - print("Testing spython conversion from singularity2docker") from spython.main.parse.parsers import SingularityParser from spython.main.parse.writers import DockerWriter diff --git a/spython/utils/terminal.py b/spython/utils/terminal.py index 7d0a7d4..0fee5c2 100644 --- a/spython/utils/terminal.py +++ b/spython/utils/terminal.py @@ -122,12 +122,16 @@ 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"]: - bot.exit("Invalid output type %s. Must be stderr or stdout." % output_type) + if output_type not in ["stdout", "stderr", "both"]: + bot.exit( + "Invalid output type %s. Must be stderr, stdout or both." % output_type + ) cmd = _process_sudo_cmd(cmd, sudo, sudo_options) + stderr_pipe = subprocess.STDOUT if output_type == "both" else subprocess.PIPE + process = subprocess.Popen( - cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True + cmd, stdout=subprocess.PIPE, stderr=stderr_pipe, universal_newlines=True ) # Allow the runner to choose streaming output or error @@ -158,7 +162,6 @@ def run_command( environ=None, background=False, ): - """ run_command uses subprocess to send a command to the terminal. If capture is True, we use the parent stdout, so the progress bar (and diff --git a/spython/version.py b/spython/version.py index d670c43..bfb48a0 100644 --- a/spython/version.py +++ b/spython/version.py @@ -5,7 +5,7 @@ # with this file, You can obtain one at http://mozilla.org/MPL/2.0/. -__version__ = "0.3.0" +__version__ = "0.3.1" AUTHOR = "Vanessa Sochat" AUTHOR_EMAIL = "vsoch@users.noreply.github.com" NAME = "spython"