Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Put application constraints on container addMachine changes #29
Conversation
added some commits
Dec 15, 2016
|
Refer to this link for build results (access rights to CI server needed): |
|
LGTM. passing through the application so that the constraints gets passed through seems good, but I'll happily defer to an expert. |
frankban
approved these changes
Dec 15, 2016
This looks good, thanks for fixing that bug! So I guess the bug was not considering that the machine is created before the unit is added in the case the location is explicit, which means application defined constraints must be propagated.
| @@ -390,12 +390,14 @@ var fromDataTests = []struct { | ||
| ContainerType: "lxc", | ||
| Series: "trusty", | ||
| ParentId: "$addMachines-6", | ||
| + Constraints: "cpu-cores=4 cpu-power=42", |
frankban
Dec 15, 2016
Member
We could a test for when constraints are specified on the application and the unit is located to "new".
| @@ -181,7 +181,7 @@ func handleUnits(add func(Change), services map[string]*charm.ApplicationSpec, a | ||
| } | ||
| // Generate the changes required in order to place this unit, and | ||
| // retrieve the identifier of the parent change. | ||
| - parentId := unitParent(add, p, records, addedMachines, servicePlacedUnits, getSeries(application, defaultSeries)) | ||
| + parentId := unitParent(add, p, records, addedMachines, servicePlacedUnits, getSeries(application, defaultSeries), application) |
frankban
Dec 15, 2016
Member
Here, and in all subsequent calls, we could just pass application.Constraints directly rather than the whole spec. [shrug]
added some commits
Dec 15, 2016
|
Refer to this link for build results (access rights to CI server needed): |
|
$$merge$$ |
|
|
|
Status: merge request accepted. Url: http://ci-gce.jujugui.org:8080/job/bundlechanges-merge |
voidspace commentedDec 15, 2016
Fixes bug #1626597 in Juju. Application constraints are added to the add machine changes for containers.
There was an existing test asserting that this was not done as that was the previous specification. This is a specification change, so I have changed the existing test rather than needing to add a new one.