Adding an option to fail upon unset env variables

[maslennikov]

Hi! First of all, thanks for the great work, the bash scripting of this project is a joy to work with 🥇

I want to use the mustache templating inside my CI to generate some build configs based on environment variables acquired from the earlier build steps, and I need to be sure that files I generate contain consistent data. As a solution, I implemented the --fail-not-set flag since the default behavior was to silently skip the unset variable and go on.

[fidian]

Beautiful commit. I have two comments. First, the .gitignore entry you added for node_modules should have a / at the end. We want to skip a node_modules/ folder. In the weird situation that it's a file, I want to know about it.

The second change is with the following line:

if [[ -z "${MO_FAIL_ON_UNSET+x}" ]] || moTest "$1"; then

Why did you use +x in there? It would either be unset/empty or it would be the string true. I don't see the advantage of changing it to be an "x".

[maslennikov]

Hi @fidian thanks for quick feedback! Since your review I fixed the thing for partials - the return code was not propagated through nested subshells in case when files were passed in as arguments.

Regarding .gitignore - fixed it, after review it can be squashed.

Regarding [[ -z "${MO_FAIL_ON_UNSET+x}" ]], I think it is not very intuitive indeed, changing it to a simpler check. Again, if it's a good-to-go, I'll squash it.

[fidian]

It's good for me! I really appreciate that you write tests to cover the code changes.

I also don't require squashing. I'll let you squash if you want to preserve dignity or otherwise would feel better. In the morning I will merge this.

[maslennikov]

Thanks Tyler! I groomed the commits a litle and now feel better :)

[fidian]

Sorry, I didn't think about this until now, but moTest will return 1 if the environment variable is empty.

Example:

echo "Words {{WORDS}}, empty {{EMPTY}}" | WORDS="words" EMPTY="" ./mo --fail-not-set

Result:

Words words, empty Env variable not set: EMPTY

So we have one of the following issues. Either the documentation is misleading, because this fails on set environment variables, or the functionality is broken and should use different variable detection functionality.

I have another repository that has code to detect if a variable is set.

If you think that having an empty string means it is not set, then the documentation should say --fail-empty and remove references to "set".

If you choose the second option, then I suggest you add a function like what I've included at the end of this message and use it instead of moTest.

You could also head down a third route and make --fail-not-set use moIsVarSet (below) and add --fail-empty use the code you've already written.

All three of those options are viable in my opinion. I suspect the last one is the best, but I leave it up to you for what you want to take on with this pull request.

# Determine if a variable is assigned, even if it is assigned an empty value.
#
# $1 - Variable name to check.
#
# Returns true (0) if the variable is set, 1 if the variable is unset.
moIsVarSet() {
    [[ "${!1-a}" == "${!1-b}" ]]
}

[fidian]

Another option is to leave the pull request as-is and I will take on the chore of the third option. I just can't do it today but could get to it next week.

[maslennikov]

Nice catch! Sorry, my bad, there was even a docstring above moTest telling it checks emptyness. I am submitting a fix for the scenario (3) with test cases adjusted accordingly.

[fidian]

Many thanks for the update! Merged!