Add security group creation to context_setup

[goonzoid]

Without this, our test apps can't see our service instances. Not sure why this isn't a problem for the MySQL service, which is currently using this.

We also removed an unused TimoutScale field on the IntegrationConfig type, having lost a good 20 minutes trying to work out why our tests were timing out immediately!

This change shouldn't break the package for any current users.

[cfdreddbot]

Hey goonzoid!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/89182176.

[robdimsdale]

FYI the reason MySQL (and Riak-CS) services don't need to set up security groups is that they exist by default on bosh-lite, and we've already set them up on our other CI environments (dijon, A1 etc).

[crhino]

@goonzoid Is there a reason you can't do something similar to Services/Data and have your security groups already prepared on your environment for when you're testing?

@crhino & @zaksoup, CF Runtime Team

[goonzoid]

@crhino @zaksoup We don't control the environments that this runs on. It's used for smoke test errands as part of our tiles, and as such will be running in customer environments and the like, so we can't assume anything about them.

[zaksoup]

@goonzoid We're happy to pull this change in. Our one concern is setting up the open security group as part of the global setup, rather than specifically for the tests that you know will need it. We've run into situations before on runtime where we make a change like this globally and later on it masks some regression that should be failing our build but doesn't.

If you're not concerned with that let us know and we'll pull this in.

@crhino + me CF Runtime Team

[goonzoid]

@zaksoup @crhino sorry for the slow response. I've been away on vacation.

We're not too worried about the case you mention. I can't imagine a context in which we'd need to test services without these security groups set up, since that basically makes our services unusable via CF.

Please merge if you're still happy to. Thanks! 😺

[zrob]

@goonzoid it looks like this more properly belongs in the services/context_setup/environment.go setup than the context setup.

@robdimsdale since this is in the services section of the helpers can your team take over this PR?

thanks,
@zrob & @DanLavine

[goonzoid]

@zrob I'm not sure I agree: Security Groups exist at the org/space level, so it makes sense to me that the lifecycle for them is tied to the lifecycle for orgs and spaces. I'm not super clear on what the alternative would be though, so perhaps you can elaborate on why this might belong in environment.go?

cc @robdimsdale

[karlkfi]

config.TimeoutScale is used by the mysql & riakcs acceptance test packages. They set context_setup.TimeoutScale to the value configured by the user, which is then used by context_setup.ScaledTimeout.

You probably set it to 0 and it multiplied all timeouts by zero, which caused them to time out immediately. The default is 1 in both the mysql and riakcs acceptance tests.

Deleting TimeoutScale is not an option. in fact, we were planning to expand its usage to all CF and curl commands in cf-test-helpers.

I can understand why you were confused tho. The cf-test-helpers doesn't load or use config.TimeoutScale internally, primarily because the load functions are not abstract enough to be extended and are instead rewritten by every package that uses them. Example

It looks like this has also been bouncing around long enough that it's no longer mergable, either. The command runner has been abstracted recently. We no longer use Eventually, because it makes for poor error messages.

[goonzoid]

You probably set it to 0 and it multiplied all timeouts by zero, which caused them to time out immediately. The default is 1 in both the mysql and riakcs acceptance tests.

The opposite is true. We set IntegrationConfig.TimeoutScale to 1, and then spent a while being confused about why everything timed out instantly. I'm not sure what the motivation for the example you linked to is, but it seems strange to me to set a field in a struct of a type defined by some package, only to use that field later to set another variable in the same package. It's not a showstopper for us though, we can just ignore the field.

It looks like this has also been bouncing around long enough that it's no longer mergable, either. The command runner has been abstracted recently. We no longer use Eventually, because it makes for poor error messages.

What's the best way for us to get this change in then? I'm happy to rebase and re-work our feature to work with whatever has changed, but I don't want to spend time on that if there's a risk that this will just bounce about unmerged for another few weeks, at which point we'll be back in the same situation. How do we make sure this doesn't happen again?

[karlkfi]

Ah, I see why everything timed out instantly for you. The global variable has an undefined default value, and the zero value is zero. The defaults are apparently all in MySQL/RiakCS test code, which obviously doesn't help you. I can't speak for the 'logic' behind that choice.

Since I already have some ideas on moving defaulting and load code into context_setup how about I take a stab at that first and then you can rebase your changes against that.

[karlkfi]

Ok, we pushed a fairly large refactor to make all cf commands use the new runner wrapper and scaled timeouts.

The Environment has been merged with Config and ValidateConfig has been separated from LoadConfig to allow loading a config with an embedded config and then chaining validations together.

RiakCS acceptance tests probably have the cleanest example of use now:
https://github.com/cloudfoundry-incubator/riakcs-acceptance-tests/blob/master/helpers/config.go
https://github.com/cloudfoundry-incubator/riakcs-acceptance-tests/blob/master/riak-cs-service/init_test.go

MySQL acceptance tests have also been updated.