Local apt defaults #621
Local apt defaults #621
Conversation
|
All tests are passing on my machine. |
| // Since Juju's state machine is currently the host machine | ||
| // for local providers, don't stomp on it. | ||
| setIfNot("enable-os-refresh-update", false) | ||
| setIfNot("enable-os-upgrade", false) |
There was a problem hiding this comment.
Don't we only want to disable this for just the bootstrap node?
This looks like you are setting it for the entire environment.
kat-co
force-pushed
the
local-apt-defaults
branch
from
0fb7154
to
5ec345e
Compare
| // Since Juju's state machine is currently the host machine | ||
| // for local providers, don't stomp on it. | ||
| cfgAttrs := env.config.AllAttrs() | ||
| if _, ok := cfgAttrs["enable-os-refresh-update"]; !ok { |
There was a problem hiding this comment.
Why not pull the attr value out of the map here and then use it directly below? Instead of the env.config.EnableOSRefreshUpdate() call.
kat-co
force-pushed
the
local-apt-defaults
branch
from
5ec345e
to
c310426
Compare
| logger.Infof("local provider; disabling refreshing OS updates.") | ||
| mcfg.EnableOSRefreshUpdate = false | ||
| } else { | ||
| mcfg.EnableOSRefreshUpdate = val.(bool) |
There was a problem hiding this comment.
My preference would be for the .(bool) to go up to the map access:
val, ok := cfgAttrs["enable-os-refresh-update"].(bool)
This will avoid any panics due to cast errors, and sort of fails safe because if there's a problem, we set update to false
kat-co
force-pushed
the
local-apt-defaults
branch
from
c310426
to
5634c4a
Compare
| s.testBootstrap(c, minCfg) | ||
|
|
||
| // Test that overrides work. | ||
| minCfg, err := minCfg.Apply(map[string]interface{}{ |
There was a problem hiding this comment.
Won't this assign to a new minCfg object? Not sure, but it could be that the mockFinish method will still be using the original minCfg values
There was a problem hiding this comment.
kat-co
force-pushed
the
local-apt-defaults
branch
from
5634c4a
to
f71ac7a
Compare
|
LGTM |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Local apt defaults For @howbazaar to correct local environment update/upgrade behavior when bootstrapping. Running all tests locally now. I won't land unless they pass on my machine, but I wanted to get this up for review.
For @howbazaar to correct local environment update/upgrade behavior when bootstrapping.
Running all tests locally now. I won't land unless they pass on my machine, but I wanted to get this up for review.