Code review comment for lp:~axwalk/juju-core/provisioningscript-api-disableaptcommands

Reviewers: mp+203866_code.launchpad.net,

Message:
Please take a look.

Description:
Add option to disable apt in ProvisioningScript

The ProvisioningScript API is updated to take a
boolean option DisablePackageCommands to completely
disable all package-related commands (apt-get, etc.)
This is for the benefit of external provisioners
that require more control over which packages are
installed and from where.

https://code.launchpad.net/~axwalk/juju-core/provisioningscript-api-disableaptcommands/+merge/203866

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/58490043/

Affected files (+41, -8 lines):
   A [revision details]
   M environs/manual/provisioner.go
   M state/api/client.go
   M state/api/params/params.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140130015505-n3kdp8ga69rxk1ty
+New revision: <email address hidden>

Index: environs/manual/provisioner.go
=== modified file 'environs/manual/provisioner.go'
--- environs/manual/provisioner.go 2014-01-28 04:58:43 +0000
+++ environs/manual/provisioner.go 2014-01-30 02:20:09 +0000
@@ -133,7 +133,10 @@

   var provisioningScript string
   if stateConn == nil {
- provisioningScript, err = client.ProvisioningScript(machineId,
machineParams.Nonce)
+ provisioningScript, err =
client.ProvisioningScript(params.ProvisioningScriptParams{
+ MachineId: machineId,
+ Nonce: machineParams.Nonce,
+ })
    if err != nil {
     return "", err
    }

Index: state/api/client.go
=== modified file 'state/api/client.go'
--- state/api/client.go 2014-01-22 03:37:36 +0000
+++ state/api/client.go 2014-01-30 02:20:09 +0000
@@ -173,11 +173,7 @@

  // ProvisioningScript returns a shell script that, when run,
  // provisions a machine agent on the machine executing the script.
-func (c *Client) ProvisioningScript(machineId, nonce string) (script
string, err error) {
- args := params.ProvisioningScriptParams{
- MachineId: machineId,
- Nonce: nonce,
- }
+func (c *Client) ProvisioningScript(args params.ProvisioningScriptParams)
(script string, err error) {
   var result params.ProvisioningScriptResult
   if err = c.st.Call("Client", "", "ProvisioningScript", args, &result);
err != nil {
    return "", err

Index: state/api/params/params.go
=== modified file 'state/api/params/params.go'
--- state/api/params/params.go 2014-01-22 03:37:36 +0000
+++ state/api/params/params.go 2014-01-30 02:20:09 +0000
@@ -520,7 +520,14 @@
  type ProvisioningScriptParams struct {
   MachineId string
   Nonce string
- DataDir string
+
+ // DataDir may be "", in which case the default will be used.
+ DataDir string
+
+ // DisablePackageCommands may be set to disable all package-related
+ // commands. It is then the responsibility of the provisioner to
+ // ensure that all the packages required by Juju are available.
+ DisablePackageCommands bool
  }

  // ProvisioningScriptResult contains the result of the

Index: state/apiserver/client/client.go
=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go 2014-01-28 04:58:43 +0000
+++ state/apiserver/client/client.go 2014-01-30 02:20:09 +0000
@@ -609,6 +609,7 @@
   if err != nil {
    return result, err
   }
+ mcfg.DisablePackageCommands = args.DisablePackageCommands
   cloudcfg := coreCloudinit.New()
   if err := cloudinit.ConfigureJuju(mcfg, cloudcfg); err != nil {
    return result, err

Index: state/apiserver/client/client_test.go
=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go 2014-01-24 14:30:55 +0000
+++ state/apiserver/client/client_test.go 2014-01-30 02:20:09 +0000
@@ -1669,7 +1669,10 @@
   // Call ProvisioningScript. Normally ProvisioningScript and
   // MachineConfig are mutually exclusive; both of them will
   // allocate a state/api password for the machine agent.
- script, err := s.APIState.Client().ProvisioningScript(machineId,
apiParams.Nonce)
+ script, err :=
s.APIState.Client().ProvisioningScript(params.ProvisioningScriptParams{
+ MachineId: machineId,
+ Nonce: apiParams.Nonce,
+ })
   c.Assert(err, gc.IsNil)
   mcfg, err := statecmd.MachineConfig(s.State, machineId,
apiParams.Nonce, "")
   c.Assert(err, gc.IsNil)
@@ -1694,6 +1697,27 @@
   }
  }

+func (s *clientSuite) TestProvisioningScriptDisablePackageCommands(c
*gc.C) {
+ apiParams := params.AddMachineParams{
+ Jobs: []params.MachineJob{params.JobHostUnits},
+ InstanceId: instance.Id("1234"),
+ Nonce: "foo",
+ HardwareCharacteristics: instance.MustParseHardware("arch=amd64"),
+ }
+ machines, err :=
s.APIState.Client().AddMachines([]params.AddMachineParams{apiParams})
+ c.Assert(err, gc.IsNil)
+ c.Assert(len(machines), gc.Equals, 1)
+ machineId := machines[0].Machine
+ script, err :=
s.APIState.Client().ProvisioningScript(params.ProvisioningScriptParams{
+ MachineId: machineId,
+ Nonce: apiParams.Nonce,
+ DisablePackageCommands: true,
+ })
+ c.Assert(err, gc.IsNil)
+ // We disabled package commands: there should be no "apt" commands in the
script.
+ c.Assert(script, gc.Not(jc.Contains), "apt-get")
+}
+
  func (s *clientSuite)
TestClientAuthorizeStoreOnDeployServiceSetCharmAndAddCharm(c *gc.C) {
   store, restore := makeMockCharmStore()
   defer restore()