No longer only search for apbs that end with -apb

[rthallisey]

Describe what this PR does and why we need it:
Give full respect to the whitelist config value by no longer filtering for '-apb'.

Changes proposed in this pull request

• Remove hardcoded filter on the suffix '-apb'

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #685

[rthallisey]

Turns out we do skip if we don't find the apb label.

        if conf.Config.Label.Spec == "" {
                log.Infof("Didn't find encoded Spec label. Assuming image is not APB and skiping")
                return nil, nil
        }

[maleck13]

LGTM

[jmrodri]

What was the premise behind this change? Do we know if there is any performance impact with this?
Can we use the white list and just add another name to include? This seems to cause my spidey senses to tingle but I may be over reacting.

[jmrodri]

Just using the ansibleplaybookbundle repo, I don't see any time difference in bootstrapping the broker.
With this change it took 5.674 seconds.

[2018-02-01T21:47:31.829Z] [INFO] - Attempting bootstrap...
[2018-02-01T21:47:37.503Z] [NOTICE] - Broker successfully bootstrapped on startup

Using the code in master, bootstrap took: 6.27 seconds

[2018-02-01T21:53:25.571Z] [INFO] - Attempting bootstrap...
[2018-02-01T21:53:31.841Z] [NOTICE] - Broker successfully bootstrapped on startup

I only did one run and captured the logs for each. Surprisingly this change came in faster by 0.596 seconds. :)

[jmrodri]

I'd like for us to look at cleaning up our logging for bootstrapping to make it more organized, possibly for a later PR.

[jmrodri]

Would it be bad to make this behavior configurable? that is disable the -apb check by default, but allow someone to turn it on with a flag? I mean this has served us well so far while removing it will solve one use case, I can see someone wanting to keep the original behavior.

[maleck13]

Forgive the lack of knowledge here but does the white_list not provide them with this functionality?

[rthallisey]

Would it be bad to make this behavior configurable? that is disable the -apb check by default, but allow someone to turn it on with a flag? I mean this has served us well so far while removing it will solve one use case, I can see someone wanting to keep the original behavior.

It's already configurable with whitelist and the default behaviour is identical to the filter I'm removing. https://github.com/openshift/ansible-service-broker/blob/master/templates/deploy-ansible-service-broker.template.yaml#L311-L312.

What I'm doing with this patch is removing the redundancy because we are filtering in two places. Whitelist should work as advertised and filter on the regex the user expects. Right now, that's not true because we always filter on the suffix -apb in addition to what's in whitelist.

@jmrodri There's some additional information in the issue #685 if I didn't address your concern. Let me know if you have any more comments/concerns.

[jmrodri]

ACK

[jmrodri]

@rthallisey thanks for that clarification. This makes a lot of sense now.

[eriknelson]

I was concerned about this change possibly introducing a regression issue over something we missed, but I honestly can't think of anything. This is a problem for APB devs, I think I helped 3 different people in the last couple weeks who didn't understand why their APBs were not showing up in the catalog.

[shawn-hurley]

I initially had concerns that the filter and pulling of metadata required that they be APBs but that does not appear to be the case.

I was also concerned that we may have wanted to change the interface here, but it seems that this change plus the change in whitelist behavior to require a whitelist value, seems to be more obvious.

👍 LGTM