juju-core

Merge lp:~axwalk/juju-core/provisioningscript-api-disableaptcommands into lp:~go-bot/juju-core/trunk

  1. provisioningscript-api-disableaptcommands
  2. Merge into trunk
Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 2277
Proposed branch: lp:~axwalk/juju-core/provisioningscript-api-disableaptcommands
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 114 lines (+45/-8)
5 files modified
environs/manual/provisioner.go (+4/-1)
state/api/client.go (+1/-5)
state/api/params/params.go (+8/-1)
state/apiserver/client/client.go (+1/-0)
state/apiserver/client/client_test.go (+31/-1)
To merge this branch: bzr merge lp:~axwalk/juju-core/provisioningscript-api-disableaptcommands
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+203866@code.launchpad.net

Commit message

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://codereview.appspot.com/58490043/

Description of the change

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://codereview.appspot.com/58490043/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (5.4 KiB)

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...

Read more...

Revision history for this message
Ian Booth (wallyworld) wrote :

LGTM with a test tweak

https://codereview.appspot.com/58490043/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/58490043/diff/1/state/apiserver/client/client_test.go#newcode1718
state/apiserver/client/client_test.go:1718: c.Assert(script,
gc.Not(jc.Contains), "apt-get")
Please add the opposite check to the test above this one to ensure
apt-get still gets inserted in the script when DisablePackageCommands is
false

https://codereview.appspot.com/58490043/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

https://codereview.appspot.com/58490043/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/58490043/diff/1/state/apiserver/client/client_test.go#newcode1718
state/apiserver/client/client_test.go:1718: c.Assert(script,
gc.Not(jc.Contains), "apt-get")
On 2014/01/30 03:07:10, wallyworld wrote:
> Please add the opposite check to the test above this one to ensure
 apt-get still
> gets inserted in the script when DisablePackageCommands is false

Done. I'd rather test the option separately, so I just modified this
test to range over []bool{false, true} and alter the checker
accordingly.

https://codereview.appspot.com/58490043/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (9.3 KiB)

The attempt to merge lp:~axwalk/juju-core/provisioningscript-api-disableaptcommands into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.210s
ok launchpad.net/juju-core/agent/tools 0.265s
ok launchpad.net/juju-core/bzr 7.281s
ok launchpad.net/juju-core/cert 3.265s
ok launchpad.net/juju-core/charm 0.590s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.034s
ok launchpad.net/juju-core/cloudinit/sshinit 1.060s
ok launchpad.net/juju-core/cmd 0.334s
ok launchpad.net/juju-core/cmd/charm-admin 0.870s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 239.801s
ok launchpad.net/juju-core/cmd/jujud 63.659s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 9.768s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container 0.039s
ok launchpad.net/juju-core/container/factory 0.053s
ok launchpad.net/juju-core/container/kvm 0.246s
ok launchpad.net/juju-core/container/kvm/mock 0.056s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.435s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.269s
ok launchpad.net/juju-core/environs 3.154s
ok launchpad.net/juju-core/environs/bootstrap 4.727s
ok launchpad.net/juju-core/environs/cloudinit 0.647s
ok launchpad.net/juju-core/environs/config 3.937s
ok launchpad.net/juju-core/environs/configstore 0.041s
ok launchpad.net/juju-core/environs/filestorage 0.032s
ok launchpad.net/juju-core/environs/httpstorage 1.002s
ok launchpad.net/juju-core/environs/imagemetadata 0.649s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.060s
ok launchpad.net/juju-core/environs/jujutest 0.240s
ok launchpad.net/juju-core/environs/manual 9.020s
ok launchpad.net/juju-core/environs/simplestreams 0.383s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.198s
ok launchpad.net/juju-core/environs/storage 1.314s
ok launchpad.net/juju-core/environs/sync 33.618s
ok launchpad.net/juju-core/environs/testing 0.235s
ok launchpad.net/juju-core/environs/tools 7.051s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.026s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 25.303s
ok launchpad.net/juju-core/juju/osenv 0.033s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.016s
ok launchpad.net/juju-core/log/syslog 0.025s
ok launchpad.net/juju-core/names 0.028s
? l...

Read more...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/manual/provisioner.go'
2--- environs/manual/provisioner.go 2014-01-28 04:58:43 +0000
3+++ environs/manual/provisioner.go 2014-01-30 03:32:35 +0000
4@@ -133,7 +133,10 @@
5
6 var provisioningScript string
7 if stateConn == nil {
8- provisioningScript, err = client.ProvisioningScript(machineId, machineParams.Nonce)
9+ provisioningScript, err = client.ProvisioningScript(params.ProvisioningScriptParams{
10+ MachineId: machineId,
11+ Nonce: machineParams.Nonce,
12+ })
13 if err != nil {
14 return "", err
15 }
16
17=== modified file 'state/api/client.go'
18--- state/api/client.go 2014-01-22 03:37:36 +0000
19+++ state/api/client.go 2014-01-30 03:32:35 +0000
20@@ -173,11 +173,7 @@
21
22 // ProvisioningScript returns a shell script that, when run,
23 // provisions a machine agent on the machine executing the script.
24-func (c *Client) ProvisioningScript(machineId, nonce string) (script string, err error) {
25- args := params.ProvisioningScriptParams{
26- MachineId: machineId,
27- Nonce: nonce,
28- }
29+func (c *Client) ProvisioningScript(args params.ProvisioningScriptParams) (script string, err error) {
30 var result params.ProvisioningScriptResult
31 if err = c.st.Call("Client", "", "ProvisioningScript", args, &result); err != nil {
32 return "", err
33
34=== modified file 'state/api/params/params.go'
35--- state/api/params/params.go 2014-01-22 03:37:36 +0000
36+++ state/api/params/params.go 2014-01-30 03:32:35 +0000
37@@ -520,7 +520,14 @@
38 type ProvisioningScriptParams struct {
39 MachineId string
40 Nonce string
41- DataDir string
42+
43+ // DataDir may be "", in which case the default will be used.
44+ DataDir string
45+
46+ // DisablePackageCommands may be set to disable all package-related
47+ // commands. It is then the responsibility of the provisioner to
48+ // ensure that all the packages required by Juju are available.
49+ DisablePackageCommands bool
50 }
51
52 // ProvisioningScriptResult contains the result of the
53
54=== modified file 'state/apiserver/client/client.go'
55--- state/apiserver/client/client.go 2014-01-28 04:58:43 +0000
56+++ state/apiserver/client/client.go 2014-01-30 03:32:35 +0000
57@@ -609,6 +609,7 @@
58 if err != nil {
59 return result, err
60 }
61+ mcfg.DisablePackageCommands = args.DisablePackageCommands
62 cloudcfg := coreCloudinit.New()
63 if err := cloudinit.ConfigureJuju(mcfg, cloudcfg); err != nil {
64 return result, err
65
66=== modified file 'state/apiserver/client/client_test.go'
67--- state/apiserver/client/client_test.go 2014-01-24 14:30:55 +0000
68+++ state/apiserver/client/client_test.go 2014-01-30 03:32:35 +0000
69@@ -1669,7 +1669,10 @@
70 // Call ProvisioningScript. Normally ProvisioningScript and
71 // MachineConfig are mutually exclusive; both of them will
72 // allocate a state/api password for the machine agent.
73- script, err := s.APIState.Client().ProvisioningScript(machineId, apiParams.Nonce)
74+ script, err := s.APIState.Client().ProvisioningScript(params.ProvisioningScriptParams{
75+ MachineId: machineId,
76+ Nonce: apiParams.Nonce,
77+ })
78 c.Assert(err, gc.IsNil)
79 mcfg, err := statecmd.MachineConfig(s.State, machineId, apiParams.Nonce, "")
80 c.Assert(err, gc.IsNil)
81@@ -1694,6 +1697,33 @@
82 }
83 }
84
85+func (s *clientSuite) TestProvisioningScriptDisablePackageCommands(c *gc.C) {
86+ apiParams := params.AddMachineParams{
87+ Jobs: []params.MachineJob{params.JobHostUnits},
88+ InstanceId: instance.Id("1234"),
89+ Nonce: "foo",
90+ HardwareCharacteristics: instance.MustParseHardware("arch=amd64"),
91+ }
92+ machines, err := s.APIState.Client().AddMachines([]params.AddMachineParams{apiParams})
93+ c.Assert(err, gc.IsNil)
94+ c.Assert(len(machines), gc.Equals, 1)
95+ machineId := machines[0].Machine
96+ for _, disable := range []bool{false, true} {
97+ script, err := s.APIState.Client().ProvisioningScript(params.ProvisioningScriptParams{
98+ MachineId: machineId,
99+ Nonce: apiParams.Nonce,
100+ DisablePackageCommands: disable,
101+ })
102+ c.Assert(err, gc.IsNil)
103+ var checker gc.Checker = jc.Contains
104+ if disable {
105+ // We disabled package commands: there should be no "apt" commands in the script.
106+ checker = gc.Not(checker)
107+ }
108+ c.Assert(script, checker, "apt-get")
109+ }
110+}
111+
112 func (s *clientSuite) TestClientAuthorizeStoreOnDeployServiceSetCharmAndAddCharm(c *gc.C) {
113 store, restore := makeMockCharmStore()
114 defer restore()

Subscribers

People subscribed via source and target branches

to status/vote changes: