juju-core

Merge lp:~thumper/juju-core/fix-uniter-proxy-env-vars into lp:~go-bot/juju-core/trunk

  1. fix-uniter-proxy-env-vars
  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: 2264
Proposed branch: lp:~thumper/juju-core/fix-uniter-proxy-env-vars
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 222 lines (+99/-36)
6 files modified
environs/config/config.go (+20/-0)
environs/config/config_test.go (+31/-0)
juju/osenv/proxy.go (+26/-18)
juju/osenv/proxy_test.go (+10/-6)
provider/local/environprovider_test.go (+6/-6)
worker/uniter/uniter_test.go (+6/-6)
To merge this branch: bzr merge lp:~thumper/juju-core/fix-uniter-proxy-env-vars
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+203459@code.launchpad.net

Commit message

Fix the proxy env vars in the uniter.

The shell variables exported in the uniter are
incorrect. They should have underscores not
dashes.

In an attempt to reduce the likelihood of this
happening again, moved the proxy code to use string
constants.

https://codereview.appspot.com/57590043/

Description of the change

Fix the proxy env vars in the uniter.

The shell variables exported in the uniter are
incorrect. They should have underscores not
dashes.

In an attempt to reduce the likelihood of this
happening again, moved the proxy code to use string
constants.

https://codereview.appspot.com/57590043/

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

Reviewers: mp+203459_code.launchpad.net,

Message:
Please take a look.

Description:
Fix the proxy env vars in the uniter.

The shell variables exported in the uniter are
incorrect. They should have underscores not
dashes.

In an attempt to reduce the likelihood of this
happening again, moved the proxy code to use string
constants.

https://code.launchpad.net/~thumper/juju-core/fix-uniter-proxy-env-vars/+merge/203459

(do not edit description out of merge proposal)

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

Affected files (+101, -36 lines):
   A [revision details]
   M environs/config/config.go
   M environs/config/config_test.go
   M juju/osenv/proxy.go
   M juju/osenv/proxy_test.go
   M provider/local/environprovider_test.go
   M worker/uniter/uniter_test.go

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

LGTM

https://codereview.appspot.com/57590043/

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

The attempt to merge lp:~thumper/juju-core/fix-uniter-proxy-env-vars into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 3.101s
ok launchpad.net/juju-core/agent/tools 0.397s
ok launchpad.net/juju-core/bzr 10.755s
ok launchpad.net/juju-core/cert 3.425s
ok launchpad.net/juju-core/charm 1.796s
? 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.033s
ok launchpad.net/juju-core/cloudinit/sshinit 1.092s
ok launchpad.net/juju-core/cmd 0.234s
ok launchpad.net/juju-core/cmd/charm-admin 0.892s
? 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 266.615s

----------------------------------------------------------------------
FAIL: machine_test.go:329: MachineSuite.TestManageEnvironRunsInstancePoller

[LOG] 72.49412 DEBUG juju.environs.configstore Making /tmp/gocheck-4751997750760398084/64/home/ubuntu/.juju/environments
[LOG] 72.66601 DEBUG juju.environs.tools reading v1.* tools
[LOG] 72.66606 INFO juju environs/testing: uploading FAKE tools 1.17.1-precise-amd64
[LOG] 72.68618 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 72.68621 DEBUG juju.environs.tools no series specified when finding tools, looking for any
[LOG] 72.68651 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:36547/dummyenv/private/tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found not found
[LOG] 72.68654 DEBUG juju.environs.simplestreams cannot load index "http://127.0.0.1:36547/dummyenv/private/tools/streams/v1/index.sjson": invalid URL "http://127.0.0.1:36547/dummyenv/private/tools/streams/v1/index.sjson" not found
[LOG] 72.68671 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:36547/dummyenv/private/tools/streams/v1/index.json": file "tools/streams/v1/index.json" not found not found
[LOG] 72.68673 DEBUG juju.environs.simplestreams cannot load index "http://127.0.0.1:36547/dummyenv/private/tools/streams/v1/index.json": invalid URL "http://127.0.0.1:36547/dummyenv/private/tools/streams/v1/index.json" not found
[LOG] 72.68771 INFO juju.environs.tools Writing tools/streams/v1/index.json
[LOG] 72.68786 INFO juju.environs.tools Writing tools/streams/v1/com.ubuntu.juju:released:tools.json
[LOG] 72.68802 INFO juju.environs.bootstrap bootstrapping environment "dummyenv"
[LOG] 72.68806 DEBUG juju.environs.bootstrap looking for bootstrap tools: series="precise", arch=<nil>, version=1.17.1
[LOG] 72.68807 INFO juju.environs.tools reading tools with major.minor version 1.17
[LOG] 72.68809 INFO juju.environs.tools filtering tools by version: 1.17.1
[LOG] 72.68809 INFO juju.environs.tools filtering tools by series: precise
[LOG] 72.68811 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 72.68816 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:36547/dummyenv/private/tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found not ...

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-01-22 23:36:28 +0000
3+++ environs/config/config.go 2014-01-28 03:57:22 +0000
4@@ -788,3 +788,23 @@
5 }
6 return repo
7 }
8+
9+// ProxyConfigMap returns a map suitable to be applied to a Config to update
10+// proxy settings.
11+func ProxyConfigMap(proxy osenv.ProxySettings) map[string]interface{} {
12+ return map[string]interface{}{
13+ "http-proxy": proxy.Http,
14+ "https-proxy": proxy.Https,
15+ "ftp-proxy": proxy.Ftp,
16+ }
17+}
18+
19+// AptProxyConfigMap returns a map suitable to be applied to a Config to update
20+// proxy settings.
21+func AptProxyConfigMap(proxy osenv.ProxySettings) map[string]interface{} {
22+ return map[string]interface{}{
23+ "apt-http-proxy": proxy.Http,
24+ "apt-https-proxy": proxy.Https,
25+ "apt-ftp-proxy": proxy.Ftp,
26+ }
27+}
28
29=== modified file 'environs/config/config_test.go'
30--- environs/config/config_test.go 2014-01-24 16:56:17 +0000
31+++ environs/config/config_test.go 2014-01-28 03:57:22 +0000
32@@ -1137,6 +1137,37 @@
33 c.Assert(config.AptFtpProxy(), gc.Equals, "")
34 }
35
36+func (*ConfigSuite) TestProxyConfigMap(c *gc.C) {
37+ defer makeFakeHome(c).Restore()
38+
39+ cfg := newTestConfig(c, testing.Attrs{})
40+ proxy := osenv.ProxySettings{
41+ Http: "http proxy",
42+ Https: "https proxy",
43+ Ftp: "ftp proxy",
44+ }
45+ cfg, err := cfg.Apply(config.ProxyConfigMap(proxy))
46+ c.Assert(err, gc.IsNil)
47+ c.Assert(cfg.ProxySettings(), gc.DeepEquals, proxy)
48+ c.Assert(cfg.AptProxySettings(), gc.DeepEquals, proxy)
49+}
50+
51+func (*ConfigSuite) TestAptProxyConfigMap(c *gc.C) {
52+ defer makeFakeHome(c).Restore()
53+
54+ cfg := newTestConfig(c, testing.Attrs{})
55+ proxy := osenv.ProxySettings{
56+ Http: "http proxy",
57+ Https: "https proxy",
58+ Ftp: "ftp proxy",
59+ }
60+ cfg, err := cfg.Apply(config.AptProxyConfigMap(proxy))
61+ c.Assert(err, gc.IsNil)
62+ // The default proxy settings should still be empty.
63+ c.Assert(cfg.ProxySettings(), gc.DeepEquals, osenv.ProxySettings{})
64+ c.Assert(cfg.AptProxySettings(), gc.DeepEquals, proxy)
65+}
66+
67 func (*ConfigSuite) TestGenerateStateServerCertAndKey(c *gc.C) {
68 // In order to test missing certs, it checks the JUJU_HOME dir, so we need
69 // a fake home.
70
71=== modified file 'juju/osenv/proxy.go'
72--- juju/osenv/proxy.go 2014-01-26 20:07:50 +0000
73+++ juju/osenv/proxy.go 2014-01-28 03:57:22 +0000
74@@ -9,6 +9,13 @@
75 "strings"
76 )
77
78+const (
79+ // Remove the likelihood of errors by mistyping string values.
80+ http_proxy = "http_proxy"
81+ https_proxy = "https_proxy"
82+ ftp_proxy = "ftp_proxy"
83+)
84+
85 // ProxySettings holds the values for the http, https and ftp proxies found by
86 // Detect Proxies.
87 type ProxySettings struct {
88@@ -28,9 +35,9 @@
89 // DetectProxies returns the proxy settings found the environment.
90 func DetectProxies() ProxySettings {
91 return ProxySettings{
92- Http: getProxySetting("http_proxy"),
93- Https: getProxySetting("https_proxy"),
94- Ftp: getProxySetting("ftp_proxy"),
95+ Http: getProxySetting(http_proxy),
96+ Https: getProxySetting(https_proxy),
97+ Ftp: getProxySetting(ftp_proxy),
98 }
99 }
100
101@@ -47,9 +54,9 @@
102 fmt.Sprintf("export %s=%s", strings.ToUpper(proxy), value))
103 }
104 }
105- addLine("http_proxy", s.Http)
106- addLine("https_proxy", s.Https)
107- addLine("ftp_proxy", s.Ftp)
108+ addLine(http_proxy, s.Http)
109+ addLine(https_proxy, s.Https)
110+ addLine(ftp_proxy, s.Ftp)
111 return strings.Join(lines, "\n")
112 }
113
114@@ -66,9 +73,9 @@
115 fmt.Sprintf("%s=%s", strings.ToUpper(proxy), value))
116 }
117 }
118- addLine("http_proxy", s.Http)
119- addLine("https_proxy", s.Https)
120- addLine("ftp_proxy", s.Ftp)
121+ addLine(http_proxy, s.Http)
122+ addLine(https_proxy, s.Https)
123+ addLine(ftp_proxy, s.Ftp)
124 return lines
125 }
126
127@@ -76,14 +83,15 @@
128 // proxy values stored in the settings object. Both the lower-case
129 // and upper-case variants are set.
130 //
131-// http-proxy, HTTP_PROXY
132-// https-proxy, HTTPS_PROXY
133-// ftp-proxy, FTP_PROXY
134+// http_proxy, HTTP_PROXY
135+// https_proxy, HTTPS_PROXY
136+// ftp_proxy, FTP_PROXY
137 func (s *ProxySettings) SetEnvironmentValues() {
138- os.Setenv("http-proxy", s.Http)
139- os.Setenv("HTTP-PROXY", s.Http)
140- os.Setenv("https-proxy", s.Https)
141- os.Setenv("HTTPS-PROXY", s.Https)
142- os.Setenv("ftp-proxy", s.Ftp)
143- os.Setenv("FTP-PROXY", s.Ftp)
144+ setenv := func(proxy, value string) {
145+ os.Setenv(proxy, value)
146+ os.Setenv(strings.ToUpper(proxy), value)
147+ }
148+ setenv(http_proxy, s.Http)
149+ setenv(https_proxy, s.Https)
150+ setenv(ftp_proxy, s.Ftp)
151 }
152
153=== modified file 'juju/osenv/proxy_test.go'
154--- juju/osenv/proxy_test.go 2014-01-26 20:07:50 +0000
155+++ juju/osenv/proxy_test.go 2014-01-28 03:57:22 +0000
156@@ -169,10 +169,14 @@
157 }
158 proxy.SetEnvironmentValues()
159
160- c.Assert(os.Getenv("http-proxy"), gc.Equals, "http proxy")
161- c.Assert(os.Getenv("HTTP-PROXY"), gc.Equals, "http proxy")
162- c.Assert(os.Getenv("https-proxy"), gc.Equals, "https proxy")
163- c.Assert(os.Getenv("HTTPS-PROXY"), gc.Equals, "https proxy")
164- c.Assert(os.Getenv("ftp-proxy"), gc.Equals, "")
165- c.Assert(os.Getenv("FTP-PROXY"), gc.Equals, "")
166+ obtained := osenv.DetectProxies()
167+
168+ c.Assert(obtained, gc.DeepEquals, proxy)
169+
170+ c.Assert(os.Getenv("http_proxy"), gc.Equals, "http proxy")
171+ c.Assert(os.Getenv("HTTP_PROXY"), gc.Equals, "http proxy")
172+ c.Assert(os.Getenv("https_proxy"), gc.Equals, "https proxy")
173+ c.Assert(os.Getenv("HTTPS_PROXY"), gc.Equals, "https proxy")
174+ c.Assert(os.Getenv("ftp_proxy"), gc.Equals, "")
175+ c.Assert(os.Getenv("FTP_PROXY"), gc.Equals, "")
176 }
177
178=== modified file 'provider/local/environprovider_test.go'
179--- provider/local/environprovider_test.go 2014-01-23 08:27:40 +0000
180+++ provider/local/environprovider_test.go 2014-01-28 03:57:22 +0000
181@@ -46,12 +46,12 @@
182 func (s *prepareSuite) SetUpTest(c *gc.C) {
183 s.FakeHomeSuite.SetUpTest(c)
184 loggo.GetLogger("juju.provider.local").SetLogLevel(loggo.TRACE)
185- s.PatchEnvironment("http-proxy", "")
186- s.PatchEnvironment("HTTP-PROXY", "")
187- s.PatchEnvironment("https-proxy", "")
188- s.PatchEnvironment("HTTPS-PROXY", "")
189- s.PatchEnvironment("ftp-proxy", "")
190- s.PatchEnvironment("FTP-PROXY", "")
191+ s.PatchEnvironment("http_proxy", "")
192+ s.PatchEnvironment("HTTP_PROXY", "")
193+ s.PatchEnvironment("https_proxy", "")
194+ s.PatchEnvironment("HTTPS_PROXY", "")
195+ s.PatchEnvironment("ftp_proxy", "")
196+ s.PatchEnvironment("FTP_PROXY", "")
197 s.HookCommandOutput(&utils.AptCommandOutput, nil, nil)
198 }
199
200
201=== modified file 'worker/uniter/uniter_test.go'
202--- worker/uniter/uniter_test.go 2014-01-26 20:18:26 +0000
203+++ worker/uniter/uniter_test.go 2014-01-28 03:57:22 +0000
204@@ -1975,12 +1975,12 @@
205 for attempt := coretesting.LongAttempt.Start(); attempt.Next(); {
206 if ctx.uniter.GetProxyValues() == expected {
207 // Also confirm that the values were specified for the environment.
208- c.Assert(os.Getenv("http-proxy"), gc.Equals, expected.Http)
209- c.Assert(os.Getenv("HTTP-PROXY"), gc.Equals, expected.Http)
210- c.Assert(os.Getenv("https-proxy"), gc.Equals, expected.Https)
211- c.Assert(os.Getenv("HTTPS-PROXY"), gc.Equals, expected.Https)
212- c.Assert(os.Getenv("ftp-proxy"), gc.Equals, expected.Ftp)
213- c.Assert(os.Getenv("FTP-PROXY"), gc.Equals, expected.Ftp)
214+ c.Assert(os.Getenv("http_proxy"), gc.Equals, expected.Http)
215+ c.Assert(os.Getenv("HTTP_PROXY"), gc.Equals, expected.Http)
216+ c.Assert(os.Getenv("https_proxy"), gc.Equals, expected.Https)
217+ c.Assert(os.Getenv("HTTPS_PROXY"), gc.Equals, expected.Https)
218+ c.Assert(os.Getenv("ftp_proxy"), gc.Equals, expected.Ftp)
219+ c.Assert(os.Getenv("FTP_PROXY"), gc.Equals, expected.Ftp)
220 return
221 }
222 }

Subscribers

People subscribed via source and target branches

to status/vote changes: