Email delivery change failed


(Alan Lehman) #1

I’m trying to change the email delivery server on my existing discourse installation to Sparkpost. I get the following error messages running discourse-setup.

What does “unary operator expected” mean?
Do I need to do something to get past “LETSENCRYPT_ACCOUNT_EMAIL change failed.”?

Found 2GB of memory and 2 physical CPU cores
setting db_shared_buffers = 256MB
setting UNICORN_WORKERS = 4
./discourse-setup: line 297: [: =: unary operator expected

Hostname for your Discourse? [forum.xxx.org]:
Email address for admin account(s)? [forum-admin@xxx.org]:
SMTP server address? [smtp.sparkpostmail.com]:
SMTP port? [587]:
SMTP user name? [SMTP_Injection]:
SMTP password? [xxx]:

Checking your domain name . . .
forum.xxx.org resolves to xxx. Looks good!

Does this look right?
Hostname      : forum.xxx.org
Email         : forum-admin@xxx.org
SMTP address  : smtp.sparkpostmail.com
SMTP port     : 587
SMTP username : SMTP_Injection
SMTP password : xxx
Let's Encrypt :

ENTER to continue, 'n' to try again, Ctrl+C to exit:
Let's Encrypt will be enabled for
LETSENCRYPT_ACCOUNT_EMAIL change failed.
web.ssl.template.yml NOT ENABLED--was it on already?
letsencrypt.ssl.template.yml NOT ENABLED -- was it on already?

(Allen - Watchman Monitoring) #2

@alehman

Would you post your app.yml file as a gist.github.com (etc) & link it here? (redacting the password, etc, of course)


(Jay Pfaffman) #3

Hmm. Line 297 is blank. Perhaps you don’t have the latest version. Try running

git pull

before you run ./discourse_setup again.

Otherwise, you can edit containers/app.yml with nano and ./launcher rebuild app.


(Matt Palmer) #4

cough

The problem is that someone forgot to quote their potentially-empty strings. :grinning: Shell is a strange language, full of wonder and 1970s design decisions.


(Jay Pfaffman) #5

Sigh. Burned again by failing to pull upstream. Not sure why I didn’t look at github instead of my local copy.

I don’t deny that’s the problem, but I can’t duplicate the error. Here’s what sets that variable. . . .

I even tried editing app.yml and removing the SMTP username and then running ./discourse-setup and still couldn’t get the error.


(Matt Palmer) #6

Line 297 is dealing with LETSENCRYPT_ACCOUNT_EMAIL; if that config item isn’t set in the config, the egrep will return an empty string, and the subsequent transformations will also be empty, returning an empty string into the letsencrypt_account_email local variable on line 296. Then, because the variable is unquoted on line 297, it expands to

if [ = "me@example.com" ]

which causes bash to complain, because = is a binary operator but there’s no left-hand side value to work with.


(Jay Pfaffman) #7

This is evidence that I shouldn’t try to debug code while also watching television. :wink:

OK. Now I see. Thanks, @mpalmer.

standalone.yml contains this:

  #LETSENCRYPT_ACCOUNT_EMAIL: me@example.com

This error can happen only when someone edits app.yml by hand, because it never creates a nil LETSENCRYPT_ACCOUNT_EMAIL. I don’t quite know what happens to the let’s encrypt template if it’s passed a nil LETSENCRYPT_ACCOUNT_EMAIL either.

There are a few other places where I should fix a similar problem.

This is a bug and I’ll endeavor to fix it shortly. It can only happen, though, if someone edits app.yml in an unpredictable way. I’m mostly of the opinion that if you edit app.yml by hand once, then you’re expected to edit it yourself from there on out.


(Matt Palmer) #8

The easiest fix: always quote variables whenever you reference them. In the worst case, it’s equivalent to not quoting the variable; in the best case, it means you get an empty string rather than a “spooky nothing” that bash has a sad about.


(Kane York) #9

Maybe we should add shellcheck to the development workflow for launcher and discourse_setup?

Here’s all the yellow warnings on the launcher script:

Line 57:
cd "$(dirname "$0")"
^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

Line 73:
  attach_on_start="-a"
  ^-- SC2034: attach_on_start appears unused. Verify it or export it.

Line 105:
            unset ver_a[0]
                  ^-- SC2184: Quote arguments to unset so they're not glob expanded.
 
Line 107:
            unset ver_b[0]
                  ^-- SC2184: Quote arguments to unset so they're not glob expanded.

Line 149:
  test=($($docker_path --version))  # Get docker version string
        ^-- SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
 
Line 150:
  test=${test[2]//,/}  # Get version alone and strip comma if exists
  ^-- SC2178: Variable was used as an array but is now assigned a string.
 
Line 153:
  if compare_version "${docker_min_version}" "${test}"; then
                                              ^-- SC2128: Expanding an array without an index only gives the first element.
 
( several more SC2128 ... )

Line 176:
  test=($($git_path --version))  # Get git version string
        ^-- SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

Line 248:
    local templates=`cat $1 | $docker_path run $user_args --rm -i -a stdin -a stdout $image ruby -e \
                         ^-- SC2086: Double quote to prevent globbing and word splitting.
                    ^-- SC2006: Use $(..) instead of legacy `..`.
                                               ^-- SC2086: Double quote to prevent globbing and word splitting.
          ^-- SC2155: Declare and assign separately to avoid masking return values.
                         ^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
 

Line 256:
        local nested_templates=$(find_templates $template)
              ^-- SC2155: Declare and assign separately to avoid masking return values.
                                                ^-- SC2086: Double quote to prevent globbing and word 

Line 272:
    arrTemplates=(${templates// / })
                  ^-- SC2206: Quote to prevent word splitting, or split robustly with mapfile or read -a.

Line 393:
if [ -z "$command" -a -z "$config" ]; then
                   ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.

Line 406:
docker_version=($($docker_path --version))
                ^-- SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
 
Line 407:
docker_version=${test[2]//,/}
^-- SC2034: docker_version appears unused. Verify it or export it.
^-- SC2178: Variable was used as an array but is now assigned a string.

Line 571:
    $docker_path rm `cat $cidbootstrap`
                    ^-- SC2046: Quote this to prevent word splitting.
                    ^-- SC2006: Use $(..) instead of legacy `..`.
                         ^-- SC2086: Double quote to prevent globbing and word splitting.

(... a couple more quote to prevent word splitting with docker commands ...)


Line 638:
        REMOTE=$(git rev-parse @{u})
                                  ^-- SC1083: This } is literal. Check expression (missing ;/\n?) or quote it.
                                ^-- SC1083: This { is literal. Check expression (missing ;/\n?) or quote it.
 
Line 639:
        BASE=$(git merge-base @ @{u})
                                   ^-- SC1083: This } is literal. Check expression (missing ;/\n?) or quote it.
                                 ^-- SC1083: This { is literal. Check expression (missing ;/\n?) or quote it.

We have the exact error another time caught, and a red warning:

Line 557:
  if [[ ! "false" =  $update_pups ]]; then
                     ^-- SC2053: Quote the rhs of = in [[ ]] to prevent glob matching.
Line 574:
  elif [[ "$ERR" > 0 ]]; then
                 ^-- SC2071: > is for string comparisons. Use -gt instead.

(Matt Palmer) #10

I’d be fine with adding a shell linter to the workflow; maybe have a Travis job for it?


(Kane York) #11

Well, the first step of course is to fix the existing warnings - you can’t just start failing the build without fixing the problems first…