Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
provider/vsphere: separate API-facing code #7216
Conversation
anastasiamac
approved these changes
Apr 10, 2017
This looks like an awesome first-cut! Thank you for doing it \o/ It will be extremely useful for VSphere testability.
I have couple of stylistic comments but nothing that is major or can be a show-stopper.
| @@ -0,0 +1,327 @@ | ||
| +// Copyright 2015 Canonical Ltd. |
| + if err != nil { | ||
| + return nil, errors.Trace(err) | ||
| + } | ||
| + vms = append(vms, &vm) |
anastasiamac
Apr 10, 2017
Member
I've been caught out in the past where setting the size and then appending to collection will actually have some empty items...
Maybe just use vms[i] =... here, especially since you could have an index on the loop if you changed _ to i?..
axw
Apr 10, 2017
Member
vms is created with 0 len, but len(items) capacity. Nevertheless, I'm changing it - the append is a holdover from when we were filtering in memory.
| + folders, err := datacenter.Folders(ctx) | ||
| + if err != nil { | ||
| + return errors.Trace(err) | ||
| + } |
anastasiamac
Apr 10, 2017
Member
This seems to be duplicated in couple of places... Maybe re-factor into one call?
axw
Apr 10, 2017
Member
It is, but not everything that wants a Finder wants Folders; and some want both, and to use the Finder apart from the Folders. Lumping them together will make the code less clear.
| +} | ||
| + | ||
| +// UpdateVirtualMachineExtraConfig updates the "ExtraConfig" attributes | ||
| +// of the specified virtual machine. Keys with empty values will be |
anastasiamac
Apr 10, 2017
Member
A side-thought - Keys with empty values or with nil values? Is there a benefit of providing means to overwrite key values with empty?
axw
Apr 10, 2017
Member
Not at the moment, and probably never. If we find we need to, we can change this internal API.
| @@ -0,0 +1,358 @@ | ||
| +// Copyright 2015 Canonical Ltd. |
| + "path" | ||
| + "path/filepath" | ||
| + | ||
| + tomb "gopkg.in/tomb.v1" |
|
$$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 |
axw commentedApr 10, 2017
Description of change
Introduce a new internal package, internal/vsphereclient,
under the vsphere provider. This package exposes a
simplified Virtual Infrastructure API client, containing
only the bits we need in Juju.
A follow up will refactor the provider further to use
this package's Client type, rather than using
govmomi.Client directly. This will enable us to improve
the test coverage of the provider without exposing the
inner workings of the VI SDK.
Note that this code is unused in this PR. It will be
used in the follow-up.
QA steps
None (code is not used yet).
Documentation changes
None.
Bug reference
None.