Fix the condition for upstream members

[SaschaDoering]

Pull Request (PR) description

The condition which decides if we have upstream members configurred or
if we import from PuppetDB was wrong. Even if no upstream members are
defined the condition was true. As a result, you couldn't import upstream
members no matter what you configured. Sorry for that

This Pull Request (PR) fixes the following issues

Fixes GH-1274

[SaschaDoering]

Hi,

there seem to be problems installing Nginx, which is why some tests fail. I don't know how this should be related to the change I made. Also the master branch failed too, so I think this is another issue. So in my opinion, this problem is fixed. Or have I overlooked something here?

[ekohl]

I think it'd be better if we made members an Optional[...] and default to undef. That way you have explicit control over if you want to specify members or use a collector. An alternative is an additional parameter to specify the behavior but that feels redundant.

[SaschaDoering]

First, thanks for your reply. I think both solutions are valid. In both cases, I have the decision whether to define or collect the upstream members. You choose to collect in which you don't define a member, but I think that's independent of the datatype we're using. In this respect, I don't see any logical difference between the two solutions, or did I get something wrong?

[ekohl]

No, you got it right: essentially there must be a choice for the module user between members and collection. Currently you can't.

I think that changing the parameter to Optional[Nginx::UpstreamMembers] $members = undef, restores the behavior from before f0bf83a.

Could you also add a test that ensures this is possible so we don't accidentally break this again? Currently the tests are broken on a new GPG key on the passenger repository though.

[SaschaDoering]

The first version of the commit f0bf83a contained a hash with undef as default. This was changed in cause of this comment #1233 (comment).
In addition, this contradicts the documentary on https://voxpupuli.org/docs/:

Are there new params with datatype Hash or Array? If possible, they should default to empty Hash/Array instead of undef. You can also enforce the datastructure like Array[String].

But I agree, there should be a test for that. I'll try to create one

[ekohl]

Note the If possible, but here additional functionality is overloaded in it. (cc @bastelfreak)

[Dan33l]

I tested locally master branch with centos7. And i was able to reproduce failing run at same step with same messages :

  Error: Execution of '/usr/bin/yum -d 0 -e 0 -y install passenger' returned 1: Importing GPG key 0xD59097AB:
   Userid     : "packagecloud ops (production key) <[email protected]>"
   Fingerprint: 418a 7f2f b0e1 e6e7 eabf 6fe8 c2e7 3424 d590 97ab
   From       : https://packagecloud.io/gpg.key
  
  
   One of the configured repositories failed (passenger repo),
   and yum doesn't have enough cached data to continue. At this point the only
   safe thing yum can do is fail.

[ekohl]

I think we'll need the equivalent of puppetlabs/puppetlabs-apache#1848

[SaschaDoering]

Unfortunately, I didn't get to create a test for it. Because I would have to import exported resources, which, according to my information booth, is not possible. Or does anyone know a way to implement that?

[SaschaDoering]

So, all tests are OK again. If nobody knows a way to implement a test with imported resources from somewhere I think I'm finished here.

[ekohl]

https://github.com/rodjek/rspec-puppet#testing-exported-resources does mention testing exported resources, but I don't see a way to collect them. A somewhat ugly workaround for this would be creating a define:

define nginx::resource::upstream::collector (
  Optional[String] $members_tag = undef,
) {
  if $members_tag {
    Nginx::Resource::Upstream::Member <<| upstream == $name and tag == $members_tag |>>
  } else {
    Nginx::Resource::Upstream::Member <<| upstream == $name |>>
  }
}

Then in this class you do:

nginx::resource::usptream::collector { $name:
  members_tag => $members_tag,
}

This would allow you to test for it:

it { is_expected.to contain_nginx__resource__upstream__collector(name).with_members_tag(nil) }

Not sure that's worth it so I'm fine with merging without tests as well.

[Dan33l]

I think we'll need the equivalent of puppetlabs/puppetlabs-apache#1848

For easily reading history of the PR , the job was done with PR #1277

[ekohl]

Thanks!

[khaefeli]

Thanks for the fix. Could you please release it also to forge?
I lost much time because I only checked the open issues / PR and then started debugging my code, because I thought I'm on the latest 0.15.0 ;)