Set env=None if cmd.run.env dict is empty

[noelmcloughlin]

This PR resolves noisy salt.cmd.run when cmd.run.env: is passed empty dictionary.

mesg: ttyname failed: Inappropriate ioctl for device

Context is:

[INFO    ] Running state [pg_createcluster 9.5 main] at time 12:32:57.344486
[INFO    ] Executing state cmd.run for [pg_createcluster 9.5 main]
[INFO    ] Executing command 'test -f /var/lib/postgresql/9.5/main/PG_VERSION && test -f /etc/postgresql/9.5/main/postgresql.conf' as user 'root' in directory '/'
mesg: ttyname failed: Inappropriate ioctl for device
[DEBUG   ] output:
[DEBUG   ] Last command return code: [0]
[INFO    ] unless execution succeeded

Fix verified on Ubuntu.

[noelmcloughlin]

The mesg: ttyname failed: Inappropriate ioctl for device messages comes from salt.utils.vt and there is no easy solution - looks like Ubunut only issue anyway.

[vutny]

Hi @noelmcloughlin

Seems to be a workaround for a Salt's bug on Ubuntu.
According to the cmd.run state docs, the default for env parameter is exactly None.

So I think there's no need to mess with default filer and Boolean operators here, since we have cascade merging of defaults into the postgres dictionary. Just better to set it in the YAML file as the source of working defaults.

[vutny]

I suggest to edit defaults.yaml instead, like this:

# defaults.yaml
postgres:
  ...
  prepare_cluster:
    command: initdb --pgdata=/var/lib/pgsql/data
    test: test -f /var/lib/pgsql/data/PG_VERSION
    user: postgres
    env: Null  # Be aware that there's no ``None`` value exists in YAML

And just use - env: {{ postgres.prepare_cluster.env }} over here.

[noelmcloughlin]

Hi @vutny Thanks for your feedbacks. Funny null did not work either. Anyway, I have discovered my commit does not fix this issue consistently - there must be some environment issue at play - so rather than travelling down a rabbit hole (its minor issue)

.. I'll raise a bug on Salt.

My investigation suggest issue arises in modules/cmdmod.py or utils/vt.py. I tried setting cmd.run.loglevel: quiet and redirecting cmd.run.test stdin/stdout to /dev/null but that does not help. Some code fails to open a tty terminal and then tries to write to null tty?

[vutny]

Hi @noelmcloughlin .
It's pity that the fix did not resolve the issue for you.
Now your PR is good anyway, it makes things consistent and readable.

Are you using Vagrant? That error is common when trying to invoke login shell without any pty attached, like in non-interactive SSH session. Setting provisioner shell to pure bash (instead of bash -l) sometimes helps.

[noelmcloughlin]

Hey @vutny Thanks, But to clarify my last message. This commit does not work - "env: null" is throwing pyyaml error for incorrectly formatted yaml, state fails.

I can revert the commit or cancel the PR (& raise salt issue instead).

[vutny]

Hmm...
I feel like I became dissipated... I did not read the docs carefully enough.
It says:

env
  A list of environment variables to be set prior to execution. 

A list! And it definitely fails if set to None explicitly.
I doubt if it even worked before...
Maybe let's fix this right away by setting an empty list []?

[vutny]

I think this comment is no longer necessary as well 😃

[noelmcloughlin]

Hi @vutny that worked.

If okay with you, I'll remove that superfluous comment in another PR (as I cannot rebase commits on this branch off master).

Thanks!!

P.S. The warning still exists mesg: ttyname failed: Inappropriate ioctl for device

[vutny]

@noelmcloughlin , it looks good. No worries. And you could just amend (git commit --amend) your last commit after removing that comment and push --force it to this branch. Everything should look pretty.

[noelmcloughlin]

@vutny Unfortunately rebasing is disabled on branches off saltstack-formulas master. :-(

remote: error: GH003: Sorry, force-pushing to cmdrun_env=none_bug is not allowed.

[vutny]

Ah, I thought you're working in your own fork. That's more flexible.

Anyhow, let's close out this PR. Would you mind to submit new one containing only relevant changes?
Thanks.

[noelmcloughlin]

Rebased as #183 Thanks!