Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Factor out common methods #6893
Conversation
| +} | ||
| + | ||
| +// Machine describes either a host machine, or a container machine. Either way | ||
| +// *state.Machine should fulfill the interface in real code. |
| +type Machine interface { | ||
| + Id() string | ||
| + AllSpaces() (set.Strings, error) | ||
| + LinkLayerDevicesForSpaces([]string) (map[string][]*state.LinkLayerDevice, error) |
frobware
Jan 31, 2017
Contributor
The return could just be []*state.LinkLayerDevice, err. From a callers perspective order may be important. And the result can be indexed via its input []string.
| + if err != nil { | ||
| + return nil, errors.Trace(err) | ||
| + } | ||
| + logger.Debugf("container %q not qualified to a space, host machine %q is using spaces %v", |
frobware
Jan 31, 2017
Contributor
Do we want to quote the space name output as we have done elsewhere? (IIRC, we had a function to do this.)
| - return nil | ||
| +func DefineEthernetDeviceOnBridge(name string, hostBridge *LinkLayerDevice) (LinkLayerDeviceArgs, error) { | ||
| + if hostBridge.Type() != BridgeDevice { | ||
| + return LinkLayerDeviceArgs{}, errors.Errorf("hostBridge must be a Bridge Device not %q", hostBridge.Type()) |
frobware
Jan 31, 2017
Contributor
The hostBridge string doesn't seem something like a human would digest.
jameinel
Jan 31, 2017
Owner
getting this would be a programming error. Thoughts on better phrasing?
"can only create an Ethernet device on a bridge not %q"
better?
| + s.machine, err = s.State.AddMachine("quantal", state.JobHostUnits) | ||
| + c.Assert(err, jc.ErrorIsNil) | ||
| + | ||
| + //s.otherState = s.NewStateForModelNamed(c, "other-model") |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Generating tarball failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
jameinel commentedJan 31, 2017
The associated methods really didn't belong on Machine, as it was dealing with how to relate 2 machine objects (a machine with its container).
We also want to have more flexibility for setting different policies for how we will be doing bridging. This is a first step towards pulling out the rough shape of that work.
It also gives us a better surface area for injecting more configuration into the system.
Potentially this lets us write tests that don't rely on state, but to make it fast to iterate, I pulled across the Database based tests.
This is simply a refactor, none of the functionality has changed.