From f8db94386dee314d56e3b65b3cb62e2a98b3cde1 Mon Sep 17 00:00:00 2001 From: Omer Tuchfeld Date: Fri, 24 Jun 2022 18:53:28 +0200 Subject: [PATCH] `_prepare_build_container` runner calls should not output to stdout All output generated by the container runtime during the `_prepare_build_container` stage should not be included in stdout - we should redirect it to stderr, as the user is just using skipper as a wrapper to run commands like make, run etc - they don't care that as a side-effect of running their commands a container is being built, and they don't want the output from that build process to be included in the stdout for the commands they wrap with skipper. This allows users to do things like VERSION=$(skipper make get_version) without having the build process output be included in their VERSION env var. Surprisingly (to me), docker/podman build output such as: ``` Step 13/18 : ENV GOPATH=/go ---> Using cache ---> 152ad1ea9005 Step 14/18 : ENV GOCACHE=/go/.cache ---> Using cache ---> 4b6a4256c84f Step 15/18 : ENV PATH=$PATH:/usr/local/go/bin:/go/bin ---> Using cache ---> 56bee68a9cda Step 16/18 : COPY . . ``` Goes to stdout and not stderr, and this causes this kind of pollution. --- .pylintrc | 2 +- skipper/cli.py | 25 ++++++++++++++++++++----- skipper/runner.py | 10 ++++++---- tests/test_cli.py | 9 ++++++--- 4 files changed, 33 insertions(+), 13 deletions(-) diff --git a/.pylintrc b/.pylintrc index dd8b293..b3500b4 100644 --- a/.pylintrc +++ b/.pylintrc @@ -65,7 +65,7 @@ confidence= # --enable=similarities". If you want to run only the classes checker, but have # no Warning level messages displayed, use"--disable=all --enable=classes # --disable=W" -disable=import-star-module-level,old-octal-literal,oct-method,print-statement,unpacking-in-except,parameter-unpacking,backtick,old-raise-syntax,old-ne-operator,long-suffix,dict-view-method,dict-iter-method,metaclass-assignment,next-method-called,raising-string,indexing-exception,raw_input-builtin,long-builtin,file-builtin,execfile-builtin,coerce-builtin,cmp-builtin,buffer-builtin,basestring-builtin,apply-builtin,filter-builtin-not-iterating,using-cmp-argument,useless-suppression,range-builtin-not-iterating,suppressed-message,no-absolute-import,old-division,cmp-method,reload-builtin,zip-builtin-not-iterating,intern-builtin,unichr-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,input-builtin,round-builtin,hex-method,nonzero-method,map-builtin-not-iterating,missing-docstring,fixme,superfluous-parens +disable=import-star-module-level,old-octal-literal,oct-method,print-statement,unpacking-in-except,parameter-unpacking,backtick,old-raise-syntax,old-ne-operator,long-suffix,dict-view-method,dict-iter-method,metaclass-assignment,next-method-called,raising-string,indexing-exception,raw_input-builtin,long-builtin,file-builtin,execfile-builtin,coerce-builtin,cmp-builtin,buffer-builtin,basestring-builtin,apply-builtin,filter-builtin-not-iterating,using-cmp-argument,useless-suppression,range-builtin-not-iterating,suppressed-message,no-absolute-import,old-division,cmp-method,reload-builtin,zip-builtin-not-iterating,intern-builtin,unichr-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,input-builtin,round-builtin,hex-method,nonzero-method,map-builtin-not-iterating,missing-docstring,fixme,superfluous-parens,too-many-locals [REPORTS] diff --git a/skipper/cli.py b/skipper/cli.py index 6c360bd..cce94c7 100644 --- a/skipper/cli.py +++ b/skipper/cli.py @@ -378,6 +378,21 @@ def _push_to_registry(registry, fqdn_image): def _prepare_build_container(registry, image, tag, git_revision, container_context, username, password, use_cache): + def runner_run(command): + """ + All output generated by the container runtime during this stage should + not be included in stdout - we should redirect it to stderr, as the + user is just using skipper as a wrapper to run commands like make, run + etc - they don't care that as a side-effect of running their commands a + container is being built, and they don't want the output from that + build process to be included in the stdout for the commands they wrap + with skipper. + + This allows users to do things like VERSION=$(skipper make get_version) + without having the build process output be included in their VERSION + env var. + """ + return runner.run(command, stdout_to_stderr=True) if tag is not None: @@ -414,20 +429,20 @@ def _prepare_build_container(registry, image, tag, git_revision, container_conte if use_cache: cache_image = utils.generate_fqdn_image(registry, namespace=None, image=image, tag=DOCKER_TAG_FOR_CACHE) - runner.run(['pull', cache_image]) + runner_run(['pull', cache_image]) command.extend(['--cache-from', cache_image]) - if runner.run(command) != 0: + if runner_run(command) != 0: exit('Failed to build image: %(image)s' % dict(image=image)) if git_revision and not git.uncommitted_changes(): utils.logger.info("Tagging image with git revision: %(tag)s", dict(tag=tag)) - runner.run(['tag', image, tagged_image_name]) + runner_run(['tag', image, tagged_image_name]) if use_cache: cache_image = utils.generate_fqdn_image(registry, namespace=None, image=image, tag=DOCKER_TAG_FOR_CACHE) - runner.run(['tag', image, cache_image]) - runner.run(['push', cache_image]) + runner_run(['tag', image, cache_image]) + runner_run(['push', cache_image]) return image diff --git a/skipper/runner.py b/skipper/runner.py index 870683b..e490aec 100644 --- a/skipper/runner.py +++ b/skipper/runner.py @@ -17,7 +17,7 @@ def get_default_net(): # pylint: disable=too-many-arguments def run(command, fqdn_image=None, environment=None, interactive=False, name=None, net=None, publish=(), volumes=None, - workdir=None, use_cache=False, workspace=None, env_file=()): + workdir=None, use_cache=False, workspace=None, env_file=(), stdout_to_stderr=False): if not net: net = get_default_net() @@ -26,15 +26,17 @@ def run(command, fqdn_image=None, environment=None, interactive=False, name=None return _run_nested(fqdn_image, environment, command, interactive, name, net, publish, volumes, workdir, use_cache, workspace, env_file) - return _run(command) + return _run(command, stdout_to_stderr=stdout_to_stderr) -def _run(cmd_args): +def _run(cmd_args, stdout_to_stderr=False): logger = logging.getLogger('skipper') cmd = [utils.get_runtime_command()] cmd.extend(cmd_args) logger.debug(' '.join(cmd)) - proc = subprocess.Popen(cmd) + proc = (subprocess.Popen(cmd) + if not stdout_to_stderr else + subprocess.Popen(cmd, stdout=sys.stderr)) proc.wait() return proc.returncode diff --git a/tests/test_cli.py b/tests/test_cli.py index 21d696f..0abcd00 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -338,7 +338,8 @@ def test_make_without_build_container_tag_with_context(self, skipper_runner_run_ mock.call(['build', '--network=host', '-t', 'build-container-image', '-f', 'Dockerfile.build-container-image', - SKIPPER_CONF_CONTAINER_CONTEXT, '--ulimit', 'nofile=65536:65536']), + SKIPPER_CONF_CONTAINER_CONTEXT, '--ulimit', 'nofile=65536:65536'], + stdout_to_stderr=True), mock.call(['make'] + make_params, fqdn_image='build-container-image', environment=[], interactive=False, name=None, net=None, publish=(), volumes=None, workdir=None, use_cache=False, workspace=None, env_file=()), @@ -1401,7 +1402,8 @@ def test_run_without_build_container_tag(self, skipper_runner_run_mock): ) expected_commands = [ mock.call(['build', '--network=host', '-t', 'build-container-image', '-f', - 'Dockerfile.build-container-image', '.', '--ulimit', 'nofile=65536:65536']), + 'Dockerfile.build-container-image', '.', '--ulimit', 'nofile=65536:65536'], + stdout_to_stderr=True), mock.call(command, fqdn_image='build-container-image', environment=[], interactive=False, name=None, net=None, publish=(), volumes=None, workdir=None, workspace=None, use_cache=False, env_file=()), @@ -1804,7 +1806,8 @@ def test_make_without_build_container_tag(self, skipper_runner_run_mock): ) expected_commands = [ mock.call(['build', '--network=host', '-t', 'build-container-image', '-f', - 'Dockerfile.build-container-image', '.', '--ulimit', 'nofile=65536:65536']), + 'Dockerfile.build-container-image', '.', '--ulimit', 'nofile=65536:65536'], + stdout_to_stderr=True), mock.call(['make'] + make_params, fqdn_image='build-container-image', environment=[], interactive=False, name=None, net=None, publish=(), volumes=None, workdir=None, workspace=None, use_cache=False, env_file=()),