Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Changed how AZs are determined in vsphere. #6738
Conversation
babbageclunk
approved these changes
Dec 21, 2016
I don't know anything about vSphere but this looks great to me!
| + } | ||
| + logger.Infof("found %d zones: %v", len(zoneInstances), zoneInstances) | ||
| + } else { | ||
| + zoneInstances, err := AllAvailabilityZones(env) |
babbageclunk
Dec 21, 2016
Member
It looks like this function returns Zones rather than ZoneInstances - it'd be clearer to name the variable zones. (I was going to suggest that you move the name-collection outside the loop until I realised they were different types.)
| + for _, z := range zoneInstances { | ||
| + zoneNames = append(zoneNames, z.Name()) | ||
| + } | ||
| + logger.Infof("found %d zones: %v", len(zoneInstances), zoneInstances) |
jameinel
Dec 21, 2016
Owner
why print 'zoneInstances' rather than 'zoneNames' here? Given 'zoneNames' is the thing we consider important enough to return. Is there something more interesting in the instance itself?
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
perrito666 commentedDec 20, 2016
Changed how Availability Zones are found for vsphere, what we model as zones can either be clusters or hosts on vsphere and the method of determining which are available, that seemed cargo culted from gce provider, was not appliable to the current provider.
This should fix:
https://bugs.launchpad.net/juju/+bug/1649690
https://bugs.launchpad.net/juju/+bug/1650422
QA