python3-*-debian syntax error during TEST_ENABLED check

[rgschmitz1]

My actions before raising this issue

• Followed the troubleshooting guide
• Read/searched the docs
• Searched past issues

faas build fails with tox error #44 seems like it may be related

Expected Behaviour

Dockerfiles using python:3.7-slim-buster base image should skip testing when TEST_ENABLED is specified as false.

Current Behaviour

The TEST_ENABLED check is throwing a syntax error due to syntax inconsistencies between alpine (busybox) and debian (dash) implementations of the default shell. This doesn't allow the use of bypassing the test using the --build-arg "TEST_ENABLED=false" flag as suggested in the documentation.

Step 27/35 : RUN if [ "$TEST_ENABLED" == "false" ]; then     echo "skipping tests";    else     eval "$TEST_COMMAND";     fi
 ---> Running in 05bb4ca63b94
/bin/sh: 1: [: false: unexpected operator

Possible Solution

I suggest replacing the double equal (==) in the Dockerfile templates with a single equal (=) to be more POSIX friendly, this worked for me in both alpine and debian versions of the python image.

RUN if [ "$TEST_ENABLED" = "false" ]; then \

Steps to Reproduce (for bugs)

1. faas-cli template pull https://github.com/openfaas-incubator/python-flask-template
2. faas-cli new --lang python3-http-debian hello-python
3. faas-cli build -f hello-python.yml

Context

I'm a college student working on a capstone project involving deploying open-source cloud native applications in Kubernetes and doing a comparison against vendor specific solutions.

I'm using of-watchdog templates as a basis for my own custom functions and would love to contribute in any way I can.

Your Environment

• FaaS-CLI version ( Full output from: faas-cli version ):

  ___                   _____           ____
 / _ \ _ __   ___ _ __ |  ___|_ _  __ _/ ___|
| | | | '_ \ / _ \ '_ \| |_ / _` |/ _` \___ \
| |_| | |_) |  __/ | | |  _| (_| | (_| |___) |
 \___/| .__/ \___|_| |_|_|  \__,_|\__,_|____/
      |_|

CLI:
 commit:  b1c09c0243f69990b6c81a17d7337f0fd23e7542
 version: 0.14.2

• Docker version docker version (e.g. Docker 17.0.05 ):

Docker version 20.10.14, build a224086

• Are you using Docker Swarm or Kubernetes (FaaS-netes)?
  Kubernetes

• Operating System and version (e.g. Linux, Windows, MacOS):

NAME="Ubuntu"
VERSION="20.04.4 LTS (Focal Fossa)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 20.04.4 LTS"
VERSION_ID="20.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=focal
UBUNTU_CODENAME=focal

• Code example or link to GitHub repo or gist to reproduce problem:
  shared output snippet in current behavior above, let me know if a more complete log is required

• Other diagnostic information / logs from troubleshooting guide

Next steps

You may join Slack for community support.

[rgschmitz1]

Possible Solution (alternative)

As an alternative, this whole test could be condensed to a single line using a ternary operator and the shell built in true/false

RUN $TEST_ENABLED && eval "$TEST_COMMAND" || echo "skipping tests"

Instead of this.

RUN if [ "$TEST_ENABLED" == "false" ]; then \
    echo "skipping tests";\
    else \
    eval "$TEST_COMMAND"; \
    fi

[LucasRoesler]

@rgschmitz1 I think changing to a single = make sense

I am not sure about the ternary, would it hide errors when eval fails and just skip testing when the eval fails?

RUN $TEST_ENABLED && eval "$TEST_COMMAND" || echo "skipping tests"

I think the desired behavior should be
a. if explicitly skipped, print the message
b. if not skipped, eval the test command and fail if it returns and error

[rgschmitz1]

Hi Lucas, thanks for the reply.

I think the desired behavior should be
a. if explicitly skipped, print the message
b. if not skipped, eval the test command and fail if it returns and error

If I understand correctly, if eval "$TEST_COMMAND" returns an error status, this should cause the container build to exit with a error.

I definitely don't want to introduce anything that would cause issues down the road for the sake of more concise code. I can double check the ternary behavior in alpine and debian images and report back here with a gist of the results to have a full record. I'll check the results for both proposed solutions and just leave it up to you about the final patch.

[LucasRoesler]

@rgschmitz1 i have no complaints with the ternary, as long as it works :) I look forward to your results. It will be good to have a consistent and correct implementation for debian and alpine.

[rgschmitz1]

@LucasRoesler
It's been a busy week, I just managed to get to testing and generating logs for ternary. Ternary would work however not exactly how I proposed originally. Turns out the eval "$TEST_COMMAND" portion needs to come last or any failures would be masked by the echo "skipping test" command as you mentioned might be the case (shell scripts are weird).

If a ternary is implemented as follows then there didn't appear to be any issues.

[ "$TEST_ENABLED" = "false" ] && echo "skipping test" || eval "$TEST_COMMAND"

Here's a log of results and dockerfiles used to test, https://gist.github.com/rgschmitz1/1cc3f375e582433dc80191b87d2e103e

In any case just putting a single = in the conditional would work also.

[LucasRoesler]

Hi @rgschmitz1 looks good. I am happy with either version. The ternary seems to be pretty readable, especially in the Dockerfile.

Do you want to patch it?

[rgschmitz1]

Hi @LucasRoesler , I was just waiting for a formal "all good to patch" message. 👍🏻

I'll submit a pull request today, thanks!