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/maas: persist bridge script #6599
Conversation
jameinel
reviewed
Nov 23, 2016
Could we actually land this as is? I'm specifically looking at the fact that we are moving where the script is located. I suppose as part of that we're not calling it anymore. Wouldn't we have some sort of functional CI test that would fail to create containers on MAAS in that situation?
| @@ -8,10 +8,7 @@ bridgescript.go: add-juju-bridge.py Makefile | ||
| $(RM) $@ | ||
| echo -n '// This file is auto generated. Edits will be lost.\n\n' >> $@ | ||
| echo -n 'package maas\n\n' >> $@ | ||
| - echo -n '//go:generate make -q\n\n' >> $@ |
jameinel
Nov 23, 2016
Owner
Why remove this line?
Don't we need "make" in order to build this .go file to include the .py file as a string?
I suppose we don't need "import path", but I think we do want to keep the "//go:generate" line. (I believe it tells 'go build' to run 'make -q' to regenerate this file).
frobware
Nov 23, 2016
Contributor
It was never used. We don't have a top-down rule that runs go generate. If I change the script I have (for ever) just run make in the provider/maas directory, then commit those changes.
frobware
Nov 23, 2016
Contributor
The go generate mechanism never materialised in the build because, for a long time, we were stuck on versions of go that did not support go generate.
| @@ -168,14 +171,18 @@ func (*environSuite) TestNewCloudinitConfigWithDisabledNetworkManagement(c *gc.C | ||
| c.Assert(cloudcfg.RunCmds(), jc.DeepEquals, expectedCloudinitConfig) | ||
| } | ||
| -func (*environSuite) TestRenderEtcNetworkInterfacesScriptMultipleNames(c *gc.C) { | ||
| - script := maas.RenderEtcNetworkInterfacesScript("eth0", "eth0:1", "eth2", "eth1") | ||
| +func (*environSuite) xTestRenderEtcNetworkInterfacesScriptMultipleNames(c *gc.C) { |
frobware
Nov 23, 2016
Contributor
Thank you. Was testing various combos by commenting them out to ensure they did something real. Reinstating.
|
Also note: the script is still executed, bridging interfaces that have a subnet/address as before. Only the location of the script has changed. Was /var/tmp, now /var/lib/juju. |
axw
approved these changes
Nov 24, 2016
LGTM, please provide QA steps and link a bug if there is one.
| + | ||
| + var bridgeScriptPath string | ||
| + | ||
| + bridgeScriptPath, err = bridgeScriptPathForSeries(forSeries) |
| + | ||
| + bridgeScriptPath, err = bridgeScriptPathForSeries(forSeries) | ||
| + if err != nil { | ||
| + return nil, err |
|
QA steps:
I did follow-up with these in a subsequent commit - they just don't show in the original commit message in GitHub. Not an existing bug; feature work (gasp!). |
|
$$retry$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
frobware commentedNov 23, 2016
Previously the MAAS script that bridges interfaces was deleted from
the filesystem after it had run because it was a one-time only event.
As we are now moving towards dynamic bridging we need permanent access
to the script; the script is now persisted in /var/lib/juju. That path
is not hard-coded, it comes from calling: paths.DataDir(series).