0k / shyaml

YAML for command line
BSD 2-Clause "Simplified" License
767 stars 57 forks source link

Empty default values with get-values-0 #57

Closed tomjn closed 4 years ago

tomjn commented 4 years ago

I'm parsing a YAML file, where a key may or may not exist, there is no way to know in advance.

Unfortunately, i'm needing to use shyaml get-values-0 to do this, and I'm unable to provide an empty default parameter

shyaml get-values-0 "sites.${VVV_SITE_NAME}.custom.wpconfig_constants" < "${VVV_CONFIG}" |
  while IFS='' read -r -d '' key &&
        IFS='' read -r -d '' value; do
      lower_value=$(echo "${value}" | awk '{print tolower($0)}')
      echo " * Adding constant '${key}' with value '${value}' to wp-config.php"
      if [ "${lower_value}" == "true" ] || [ "${lower_value}" == "false" ] || [[ "${lower_value}" =~ ^[+-]?[0-9]*$ ]] || [[ "${lower_value}" =~ ^[+-]?[0-9]+\.?[0-9]*$ ]]; then
        noroot wp config set "${key}" "${value}" --raw
      else
        noroot wp config set "${key}" "${value}"
      fi
  done

As this is an optional section, and a new feature, most VVV YAML configs in existence do not have a wp_config_constants section.

As a result, the majority of sites fail to provision due to the bad return code of shyaml. Passing -q just makes it impossible to tell from the output that it was shyaml that triggered the problem

How do I pass an appropriate empty/null/non-op value as a default?

vaab commented 4 years ago

I may not have understood your concern yet, but did you know that you could provide a default value if the path doesn't exist ? Would that solve your issue ?

Of course get-values-0 alone will have problems using this default value.

You mention 'bad' return code of shyaml. I'm not sure what you mean by that ? shyaml will return errorlevel 1 if it hits a undefined key error, passing -q will not change this.

So I would simply suggest you to do something equivalent:

if wpconfig_constants=$(shyaml get-value -q -y sites.${VVV_SITE_NAME}.custom.wpconfig_constants < "${VVV_CONFIG}"); then
    while IFS='' read -r -d '' key &&
          IFS='' read -r -d '' value; do
        lower_value=$(echo "${value}" | awk '{print tolower($0)}')
        echo " * Adding constant '${key}' with value '${value}' to wp-config.php"
        if [ "${lower_value}" == "true" ] || [ "${lower_value}" == "false" ] || [[ "${lower_value}" =~ ^[+-]?[0-9]*$ ]] || [[ "${lower_value}" =~ ^[+-]?[0-9]+\.?[0-9]*$ ]]; then
          noroot wp config set "${key}" "${value}" --raw
        else
          noroot wp config set "${key}" "${value}"
        fi
    done < <(printf "%s" "$wpconfig_constants" | shyaml get-values-0)
fi

Would that solve your issue ?

tomjn commented 4 years ago

I may not have understood your concern yet, but did you know that you could provide a default value if the path doesn't exist ? Would that solve your issue ?

Yes but as far as i can tell there is no documented value for an empty value. I attempted numerous values, empty strings, empty bash arrays, all failed, it was incredibly frustrating. I'd recommend updating the documentation to indicate how this can be done, or if it can't

You mention 'bad' return code of shyaml. I'm not sure what you mean by that ? shyaml will return errorlevel 1 if it hits a undefined key error, passing -q will not change this.

That's exactly the problem, shyaml returns an exit code with a value of 1, aka an error. My bash scripts are setup to fail when such a situation occurs via set -eo pipefail

I will need to test the code snippet, but I'm unsure how this avoids the problem, as if the section does not exist, shyaml get-values-0 will still return an exit code of 1, causing the scripts to fail

vaab commented 4 years ago

Yes but as far as i can tell there is no documented value for an empty value. I attempted numerous values, empty strings, empty bash arrays, all failed, it was incredibly frustrating. I'd recommend updating the documentation to indicate how this can be done, or if it can't

I'm always grateful for feedbacks and improvements suggestions to the docs. In what way the doc, more specifically the section I've linked in my previous message, could be improved ? I'm trying to understand what you were looking for when you write "there is no documented value for an empty value". Might it be: there is no documentation on how to provide an empty default value in case of missing key ? If yes, there are probably some misunderstanding : shyaml input and output strings. It interprets input string as YAML, but output is not always YAML (use -y to ensure that). You are free to output the default value of 'null', and make sure to interpret it as YAML if this is what you need.

That's exactly the problem, shyaml returns an exit code with a value of 1, aka an error. My bash scripts are setup to fail when such a situation occurs via set -eo pipefail

Well, that's a good practice, but this should not block you to use shell commands that will use errorlevel 1 to communicate with your script. Bash will avoid exiting if you use a command that fails in a if statement for instance.

I will need to test the code snippet, but I'm unsure how this avoids the problem, as if the section does not exist, shyaml get-values-0 will still return an exit code of 1, causing the scripts to fail

Well, this is why it is enclosed in a if block.

Tell me if any if this helps, or misses your point. Thanks for your feedback.

tomjn commented 4 years ago

Might it be: there is no documentation on how to provide an empty default value in case of missing key ?

Yes!! The structure may or may not exist! If it doesn't exist it's the same as if it existed but with no values.

For example here is some pseudocode:

my_colours = [ "red", "yellow", "blue" ]
your_colours = []
foreach list of colours
    print the colour

In this case, if wpconfig_constants exists and contains options, I need to handle them, which I do. I would expect to see red yellow and blue, then have no further output as the second list is empty. This is not the case when using shyaml as it complains. There is no way to provide an empty/blank/null value.

But if it does not exist, it fails because it does not exist, and I cannot test for this.

So I can provide a default value, but what is that default value?

I hadn't considered that I needed to pass a YAML string in as the default value.

You are free to output the default value of 'null', and make sure to interpret it as YAML if this is what you need.

vagrant@vvv:~$ shyaml get-values-0 "sites.wordpress-two.custom.wpconfig_constants" "null" < "/vagrant/config.yml"
Error: get-values-0 does not support 'str' type. Please provide or select a sequence or struct.
vagrant@vvv:~$ 

Nope, does not work, "null" is not a valid default value. Neither is null ~ or "".

For example, here is what is in config.yml:

sites:

  wordpress-two:
    skip_provisioning: false
    description: "A standard WP install, useful for building plugins, testing things, etc"
    repo: https://github.com/Varying-Vagrant-Vagrants/custom-site-template.git
    hosts:
      - two.wordpress.test

So if we have this:

sites:
  wordpress-two:
    hosts:
      - two.wordpress.test

How do I provide a default value, equivalent to this:

sites:
  wordpress-two:
    hosts:
      - two.wordpress.test
    custom:
      wpconfig_constants:

In addition, if I did need to pass in default values, would I pass in this as the default value?

  - default: value
... etc

Right now the documentation is extremely ambiguous for anything other than the most basic of uses, and the error codes are unhelpful.

For example:

Error: get-values-0 does not support 'str' type. Please provide or select a sequence or struct.

What is a sequence or struct? This makes no sense to me, has no URLs, and is not mentioned in the docs. Is it referring to a bash data type? Python data type? YAML markup?

vaab commented 4 years ago

But if it does not exist, it fails because it does not exist, and I cannot test for this.

Well, the purpose of giving you an errorlevel of 1 is a direct way to test it. And yes, you can do that even in set -eo pipefail, of course, without causing an exit of your program. Bash will not exit the shell upon failure of command in a ìf construct.

Let me repeat it for clarity: contrary to your statement, you can test for this, and you should. The code I provided to you will work even with set -eo pipefail. This is the most natural way to go.

Let me say it again: when you write about my code "if the section does not exist, shyaml get-values-0 will still return an exit code of 1, causing the scripts to fail" is a wrong assumption and I please ask you to do it. set -e option will not exit upon any errorlevel 1, only the one that are not part of a test (if, while, && and others).

Let me quote this from the bash help about set -e:

The shell does not exit if the command that fails is part of the command list immediately following a while or until keyword, part of the test in an if statement, part of any command executed in a && or || list except the command following the final && or ||

set -e without this would not make any sense as it would prevent you using conditional construct from bash.

As a final note, if you'd like to avoid quitting your program because of a set -e on a command that you want to let fail for good reasons, then you should maybe think of disabling temporarily the option set -e, or better, explicitely add a || true at the end of the command you want to allow to fail.

If you want to avoid the stderr message warning you about missing key, because, as I understand it, you are expecting this to happen, -q is the way to go as it'll silence shyaml stderr (but this is just a shortcut, as anyway you could do the same thing with 2>/dev/null at the end of the shyaml call).

This -q will not change the errorlevel of shyaml call, because this would remove the only way to get the information that the key doesn't exist.

Could you please test the code I handed you in my first reply, as it appears to me that it has good chances to solve your problem.

On another hand, you are right on the fact that providing a default value to get-values-0 is broken in some way, and will not work, and/or give you confusing error messages. That is a concern that I agree, should be fixed in a way or another.

The good news, is that you don't need this feature, as far as I can tell.

tomjn commented 4 years ago

@vaab I've revisited this and I see now what you mean, but it was buried under an example that refactored my loop, I couldn't see how to simply extract the answer at the time without adding a lot of complexity to every situation that required it

I believe the confusion can be sidestepped completely with a basic atomic example.

If we take example.yml:

foo:
  bar:
   - value
   - value

We can process the bar values like this, safe in the knowledge if bar is undefined we can handle that case:

if BAR=$(shyaml get-values -y -q "foo.bar" < example.yml); then
  for i in $BAR; do
    echo ${i}
  done
else
  echo "No values found"
fi

The same works for keys etc