Handle array in custom prepend/append configuration. (2)

[SteveMaddison]

No description provided.

[bastelfreak]

Hi @SteveMaddison, thanks for the PR. Are you able to provide spec tests as well?

[SteveMaddison]

I'm afraid I had too many issues running tests locally to make any progress there. This is a redo of a (since closed) PR from more than a year ago, which couldn't be merged due to conflicts. It would however seem that this change is incompatible with tests which have been added in the mean time. It's not something I require any more, so feel free to close this PR.

[wyardley]

@SteveMaddison: I know what you mean. It took me a while to get to where I could get the tests running properly locally... tho the steps in:
https://github.com/voxpupuli/puppet-nginx/blob/master/.github/CONTRIBUTING.md
(btw, doing it in a feature branch instead of master should make rebasing or creating other features much easier later on).

Unfortunately, this one is currently failing tests, so we can't merge it until there are tests for the feature and / or existing tests are adjusted accordingly.

Specifically, it's failing on this one (though I think technically it's a list of lists, not a list of hashes here); I'll look at it some more later, but I think something actually is broken, maybe because you're not iterating through a second level list if it exists:

          title: 'should contain duplicate appended directives from list of hashes',
          attr: 'http_cfg_prepend',
          value: [['allow', 'test value 1'], ['allow', 'test value 2']],
          match: [
            '  allow test value 1;',
            '  allow test value 2;'
          ]
        },

From a quick reading, it seems as if these may already work whether it's an array or a hash:
https://github.com/voxpupuli/puppet-nginx/blob/master/templates/conf.d/nginx.conf.erb#L23-L31
https://github.com/voxpupuli/puppet-nginx/blob/master/templates/conf.d/nginx.conf.erb#L174-L177

[wyardley]

@SteveMaddison:
So does seem like it will take an array of arrays as an argument as implemented already:
https://github.com/voxpupuli/puppet-nginx/blob/master/spec/classes/nginx_spec.rb#L768-L793
that is [['foo', 'bar'], ['baz', 'example']]. That is a bit confusing, and maybe not similar to how the rest of the module is implemented, however, I think if you are going to update this, you need to add a new test case for the case where there's a flat array, and keep it working in the example between L778-L784 (which means you'll need to write it in such a way that it's not too hard to read).

If you're willing to specify your array as the module expects, this could just be a docs fix (i.e., once the main class is actually documented, we can specify the format that it expects). I do agree that this is not totally consistent with the way the rest of the module works, but it might actually be a better structure to use the existing examples, that is:

1. hash ({ 'test1' => 'test value 1', 'test2' => 'test value 2', 'allow' => 'test value 3' }
2. array of arrays ([['allow', 'test value 1'], ['allow', 'test value 2']])
3. hash of arrays ({ 'test1' => ['test value 1', 'test value 2', 'test value 3'] })

I personally think #2 is, in a way, better than just a flat array.

One other note, may be a recent change, but there is both http_cfg_append and http_cfg_prepend now.

[SteveMaddison]

@wyardley I think there are indeed enough options without resorting to a flat array. I don't remember the exact use case this change was meant to address but users always have the option to convert a flat array to an existing compatible structure before passing to the module. I therefore don't see any real need to merge this any more, so I'll just close this PR.