command checks should have exec as attribute, not as key

[sshipway]

Currently, command checks are defined like this:

command:
  "/bin/true":
    exit_code: 0

however, this means that it is very hard, if not impossible, to remove checks when doing a deepmerge of the hiera hash definition (as you do in automation systems such as Puppet), as deepmerge does not provide a way to prune hashes, only lists. It would be much, much more helpful to be able to define checks like this:

command:
  someuniquename:
    exec: "/bin/true"
    exit_code: 0
    skip: false

This would allow you to have a much simpler key ("someuniquename"), and also to potentially override the command only at a lower level. Also, by having the 'skip' attribute, it allows individual commands to be selectively disabled in a deepmerge (when multiple hashes are merged by a configuration management tool to generate the final configuration).

Indeed, having the 'skip' attribute (defaulting to 'true') on every test would be a Good Thing.

It should be pretty easy to make this change backwards compatible, simply by making the 'exec' attribute default to the someuniquename key.

[sshipway]

I changed the additional attribute name to 'enable_test' as 'enabled' is already in use for service checks.

[sshipway]

I now have a pull request for this:
#434

This uses a new attribute 'skip' to leverage the existing skip feature and effectively optionally disable checks

[aelsabbahy]

Can you explain the deepmerge scenario further? I understand the code/format change.. I also understand some of the value in having a skip attribute, but I'm not understanding the benefit of the exec attribute.

[sshipway]

Sure.

Since Goss uses a yaml format for configurations, this matches well with configuration management tools, and the Puppet way of managing yaml in a heiarachy (see heira). This allows us to set up, for example, heirachies of host definitions, with some tests (e.g. ssh running) done at a top level, some done at a 'server class' level, and others at a 'host' level. These are then merged together to form the final goss file, in a 'deepmerge' where both hashes are merged as closely as possible.

The problem comes when we want to disable a test at a lower level when it was defined higher up. Deep merge does not have any real way to do this :(, though you can replace attributes.

The two changes I'm proposing will address this scenario, and make gossfiles easier to read.

By having the command moved to an attribute, it means that lower merged layers can then override the command attribute but keep everything else the same.

By having 'skip' as an attribute, individual checks can be disabled at a lower level even if they were defined at a higher level.

By having the 'exec' attribute, we make it possible for the test command to be overridden at a lower level while keeping the rest of the configuration. Again, not so much difference for goss itself, but very helpful for configuration management systems that want to easily configure the goss.yaml

Although this does not make much difference to goss itself, it means that the configuration files become much easier to manage with a configuration management tool, particularly if you are managing a large number of hosts in heirachial groups where tests are common across subsets of hosts.

[sshipway]

I notice that #150 refers to merging included gossfiles in a fixed order, but it does not do 'deepmerging' that #392 asks for. Deepmerging in this case would be the same as our configuration management tool deepmerge and would benefit from the same features.

Currently, if you you override a particular command definition, you have to redefine it entirely. A deepmerge would mean you only need to redefine the part being changed.

Puppet have a good doc explaining deep merges: https://puppet.com/docs/puppet/5.0/hiera_merging.html#deep
This also mentions 'hash' merges, which is (I think) the type you are using when importing gossfiles.

[sshipway]

I'm trying to fix the CI tests for this PR so that it passes Travis. Never worked with Travis before so this should be interesting.

[sshipway]

This can be closed now that pr #434 is merged