Skip to content

Commit

Permalink
_prepare_build_container runner calls should not output to stdout
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
omertuc authored and eranco74 committed Jul 6, 2022
1 parent 0f23176 commit f8db943
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 13 deletions.
2 changes: 1 addition & 1 deletion .pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
25 changes: 20 additions & 5 deletions skipper/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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

Expand Down
10 changes: 6 additions & 4 deletions skipper/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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

Expand Down
9 changes: 6 additions & 3 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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=()),
Expand Down Expand Up @@ -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=()),
Expand Down Expand Up @@ -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=()),
Expand Down

0 comments on commit f8db943

Please sign in to comment.