juju-core

Merge lp:~thumper/juju-core/proxy-config-omit into lp:~go-bot/juju-core/trunk

  1. proxy-config-omit
  2. Merge into trunk
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 2420
Proposed branch: lp:~thumper/juju-core/proxy-config-omit
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 85 lines (+24/-27)
2 files modified
environs/config/config.go (+24/-20)
environs/config/config_test.go (+0/-7)
To merge this branch: bzr merge lp:~thumper/juju-core/proxy-config-omit
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+210952@code.launchpad.net

Commit message

Omit empty proxy settings in config.

I had changed them to default to "" because there is
no way to unset the value once you have set one for
an environment. The correct fix is to implement
'juju unset-env'

https://codereview.appspot.com/75700043/

Description of the change

Omit empty proxy settings in config.

I had changed them to default to "" because there is
no way to unset the value once you have set one for
an environment. The correct fix is to implement
'juju unset-env'

https://codereview.appspot.com/75700043/

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

Reviewers: mp+210952_code.launchpad.net,

Message:
Please take a look.

Description:
Omit empty proxy settings in config.

I had changed them to default to "" because there is
no way to unset the value once you have set one for
an environment. The correct fix is to implement
'juju unset-env'

https://code.launchpad.net/~thumper/juju-core/proxy-config-omit/+merge/210952

(do not edit description out of merge proposal)

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

Affected files (+9, -16 lines):
   A [revision details]
   M environs/config/config.go
   M environs/config/config_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-20140313162936-3e8iq4csv4axq6z8
+New revision: <email address hidden>

Index: environs/config/config.go
=== modified file 'environs/config/config.go'
--- environs/config/config.go 2014-03-09 20:48:29 +0000
+++ environs/config/config.go 2014-03-14 02:10:35 +0000
@@ -733,15 +733,13 @@
   "bootstrap-retry-delay": schema.Omit,
   "bootstrap-addresses-delay": schema.Omit,
   "rsyslog-ca-cert": schema.Omit,
-
- // Proxy values default to "", otherwise they can't be set to blank.
- "http-proxy": "",
- "https-proxy": "",
- "ftp-proxy": "",
- "no-proxy": "",
- "apt-http-proxy": "",
- "apt-https-proxy": "",
- "apt-ftp-proxy": "",
+ "http-proxy": schema.Omit,
+ "https-proxy": schema.Omit,
+ "ftp-proxy": schema.Omit,
+ "no-proxy": schema.Omit,
+ "apt-http-proxy": schema.Omit,
+ "apt-https-proxy": schema.Omit,
+ "apt-ftp-proxy": schema.Omit,

   // Deprecated fields, retain for backwards compatibility.
   "tools-url": "",

Index: environs/config/config_test.go
=== modified file 'environs/config/config_test.go'
--- environs/config/config_test.go 2014-03-13 07:54:56 +0000
+++ environs/config/config_test.go 2014-03-14 02:10:35 +0000
@@ -1026,13 +1026,6 @@
   attrs["tools-metadata-url"] = ""
   attrs["tools-url"] = ""
   attrs["image-stream"] = ""
- attrs["http-proxy"] = ""
- attrs["https-proxy"] = ""
- attrs["ftp-proxy"] = ""
- attrs["no-proxy"] = ""
- attrs["apt-http-proxy"] = ""
- attrs["apt-https-proxy"] = ""
- attrs["apt-ftp-proxy"] = ""

   // Default firewall mode is instance
   attrs["firewall-mode"] = string(config.FwInstance)

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

On 2014/03/14 02:23:33, thumper wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/75700043/

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

On 2014/03/14 02:23:33, thumper wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/75700043/

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

The attempt to merge lp:~thumper/juju-core/proxy-config-omit into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 0.989s
ok launchpad.net/juju-core/agent/mongo 0.526s
ok launchpad.net/juju-core/agent/tools 0.211s
ok launchpad.net/juju-core/bzr 5.478s
ok launchpad.net/juju-core/cert 2.402s
ok launchpad.net/juju-core/charm 0.428s
? 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.029s
ok launchpad.net/juju-core/cloudinit/sshinit 0.778s
ok launchpad.net/juju-core/cmd 0.161s
ok launchpad.net/juju-core/cmd/charm-admin 0.773s
? 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 184.843s

----------------------------------------------------------------------
FAIL: machine_test.go:719: MachineSuite.TestMachineEnvirnWorker

[LOG] 26.32353 DEBUG juju.environs.configstore Making /tmp/gocheck-4037200794235010051/64/home/ubuntu/.juju/environments
[LOG] 26.39743 DEBUG juju.environs.tools reading v1.* tools
[LOG] 26.39748 INFO juju environs/testing: uploading FAKE tools 1.17.5-precise-amd64
[LOG] 26.41133 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 26.41134 DEBUG juju.environs.tools no series specified when finding tools, looking for any
[LOG] 26.41141 DEBUG juju.environs.simplestreams fetchData failed for "tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found not found
[LOG] 26.41143 DEBUG juju.environs.simplestreams cannot load index "streams/v1/index.sjson": invalid URL "tools/streams/v1/index.sjson" not found
[LOG] 26.41148 DEBUG juju.environs.simplestreams fetchData failed for "tools/streams/v1/index.json": file "tools/streams/v1/index.json" not found not found
[LOG] 26.41150 DEBUG juju.environs.simplestreams cannot load index "streams/v1/index.json": invalid URL "tools/streams/v1/index.json" not found
[LOG] 26.41190 INFO juju.environs.tools Writing tools/streams/v1/index.json
[LOG] 26.41196 INFO juju.environs.tools Writing tools/streams/v1/com.ubuntu.juju:released:tools.json
[LOG] 26.41199 INFO juju.environs.bootstrap bootstrapping environment "dummyenv"
[LOG] 26.41204 DEBUG juju.environs.bootstrap looking for bootstrap tools: series="precise", arch=<nil>, version=1.17.5
[LOG] 26.41205 INFO juju.environs.tools reading tools with major.minor version 1.17
[LOG] 26.41206 INFO juju.environs.tools filtering tools by version: 1.17.5
[LOG] 26.41207 INFO juju.environs.tools filtering tools by series: precise
[LOG] 26.41208 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 26.41214 DEBUG juju.environs.simplestreams fetchData failed for "tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found not found
[LOG] 26.41215 DEBUG juju.environs.simplestreams cannot load index "streams/v1/index.sjson": invalid URL "tools/streams/v1/index.sjson" not found
[LOG] 26.41229 DEBUG juju.environs.simplestreams fetchData failed for "tools/...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/config/config.go'
2--- environs/config/config.go 2014-03-09 20:48:29 +0000
3+++ environs/config/config.go 2014-03-14 03:44:33 +0000
4@@ -733,15 +733,13 @@
5 "bootstrap-retry-delay": schema.Omit,
6 "bootstrap-addresses-delay": schema.Omit,
7 "rsyslog-ca-cert": schema.Omit,
8-
9- // Proxy values default to "", otherwise they can't be set to blank.
10- "http-proxy": "",
11- "https-proxy": "",
12- "ftp-proxy": "",
13- "no-proxy": "",
14- "apt-http-proxy": "",
15- "apt-https-proxy": "",
16- "apt-ftp-proxy": "",
17+ "http-proxy": schema.Omit,
18+ "https-proxy": schema.Omit,
19+ "ftp-proxy": schema.Omit,
20+ "no-proxy": schema.Omit,
21+ "apt-http-proxy": schema.Omit,
22+ "apt-https-proxy": schema.Omit,
23+ "apt-ftp-proxy": schema.Omit,
24
25 // Deprecated fields, retain for backwards compatibility.
26 "tools-url": "",
27@@ -911,23 +909,29 @@
28 AddressesDelay time.Duration
29 }
30
31+func addIfNotEmpty(settings map[string]interface{}, key, value string) {
32+ if value != "" {
33+ settings[key] = value
34+ }
35+}
36+
37 // ProxyConfigMap returns a map suitable to be applied to a Config to update
38 // proxy settings.
39 func ProxyConfigMap(proxy osenv.ProxySettings) map[string]interface{} {
40- return map[string]interface{}{
41- "http-proxy": proxy.Http,
42- "https-proxy": proxy.Https,
43- "ftp-proxy": proxy.Ftp,
44- "no-proxy": proxy.NoProxy,
45- }
46+ settings := make(map[string]interface{})
47+ addIfNotEmpty(settings, "http-proxy", proxy.Http)
48+ addIfNotEmpty(settings, "https-proxy", proxy.Https)
49+ addIfNotEmpty(settings, "ftp-proxy", proxy.Ftp)
50+ addIfNotEmpty(settings, "no-proxy", proxy.NoProxy)
51+ return settings
52 }
53
54 // AptProxyConfigMap returns a map suitable to be applied to a Config to update
55 // proxy settings.
56 func AptProxyConfigMap(proxy osenv.ProxySettings) map[string]interface{} {
57- return map[string]interface{}{
58- "apt-http-proxy": proxy.Http,
59- "apt-https-proxy": proxy.Https,
60- "apt-ftp-proxy": proxy.Ftp,
61- }
62+ settings := make(map[string]interface{})
63+ addIfNotEmpty(settings, "apt-http-proxy", proxy.Http)
64+ addIfNotEmpty(settings, "apt-https-proxy", proxy.Https)
65+ addIfNotEmpty(settings, "apt-ftp-proxy", proxy.Ftp)
66+ return settings
67 }
68
69=== modified file 'environs/config/config_test.go'
70--- environs/config/config_test.go 2014-03-13 07:54:56 +0000
71+++ environs/config/config_test.go 2014-03-14 03:44:33 +0000
72@@ -1026,13 +1026,6 @@
73 attrs["tools-metadata-url"] = ""
74 attrs["tools-url"] = ""
75 attrs["image-stream"] = ""
76- attrs["http-proxy"] = ""
77- attrs["https-proxy"] = ""
78- attrs["ftp-proxy"] = ""
79- attrs["no-proxy"] = ""
80- attrs["apt-http-proxy"] = ""
81- attrs["apt-https-proxy"] = ""
82- attrs["apt-ftp-proxy"] = ""
83
84 // Default firewall mode is instance
85 attrs["firewall-mode"] = string(config.FwInstance)

Subscribers

People subscribed via source and target branches

to status/vote changes: