Add support for apt-no-proxy option #7119

Merged
merged 5 commits into from Mar 26, 2017

Conversation

Projects
None yet
3 participants
Member

wupeka commented Mar 17, 2017

Description of change

Add support for apt-no-proxy - disable proxy for apt for selected hosts.

QA steps

  1. juju model-config apt-no-proxy=h1,h2,h3
  2. deploy unit
  3. verify that /etc/apt/apt.config.d/95-juju-proxy-settings contains Apt::::"" "DIRECT"; lines for protocols in http, https, ftp and host in h1, h2 h3

Documentation changes

This option has to be documented in juju/docs

Bug reference

https://bugs.launchpad.net/juju/+bug/1466951

Member

wupeka commented Mar 17, 2017

!!testme!!

Member

wupeka commented Mar 17, 2017

!!test!!

Owner

jameinel commented Mar 17, 2017

Member

wupeka commented Mar 17, 2017

Currently - yes, it is a fallback if apt-*-proxy is not present and *-proxy is present.
I want to add what's in https://bugs.launchpad.net/juju/+bug/1666353 here too and fix both issues at once, I need this PR to have CI tests run.
I'll ask for review once it's ready.

Owner

jameinel commented Mar 20, 2017

!!test!!

All the changes here look sane to me, but it doesn't seem to actually set Acquire:: sort of settings. Is that all done inside the 'utils' package?

Owner

jameinel commented Mar 20, 2017


FAIL: config_test.go:1090: ConfigSuite.TestAptProxyConfigMap

config_test.go:1102:
c.Assert(cfg.AptProxySettings(), gc.DeepEquals, proxySettings)
... obtained proxy.Settings = proxy.Settings{Http:"http://httpproxy", Https:"https://httpsproxy", Ftp:"ftp://ftpproxy", NoProxy:"127.0.0.1,localhost,::1"}
... expected proxy.Settings = proxy.Settings{Http:"http://httpproxy", Https:"https://httpsproxy", Ftp:"ftp://ftpproxy", NoProxy:""}

This sort of test suite failure should be something you can iterate on without needing to use the bot. Feel free to ping me if you're confused how to run a subset of the test suite on your own machine.

Owner

jameinel commented Mar 20, 2017

I realize you might not know things like "go test -check.v -check.f REGEX" to run a subset of the tests for a particular package.

Member

wupeka commented Mar 20, 2017

!!test!!

Owner

jameinel commented Mar 23, 2017

Did you get this to a point for another review? I see that the tests have passed

Member

wupeka commented Mar 23, 2017

Yes, it is ready for review. When this one is merged I'll add PR for https://bugs.launchpad.net/juju/+bug/1666353 as it depends on it.

One minor comment tweak

The changes look good to me. Have you tried running them on a live environment?

One thing I like to do is set up "apt install apt-cacher-ng" on the machine that hosts the main MAAS server, and then configure both MAAS and Juju environments on that machine to use the proxy. It generally makes "bootstrap" and "deploy" quite a bit faster, if you're doing repeated testing. And it gives you an excuse to test the APT settings and see that they are working as expected.

environs/config/config.go
@@ -664,6 +669,11 @@ func (c *Config) AptFTPProxy() string {
return addSchemeIfMissing("ftp", c.getWithFallback(AptFTPProxyKey, FTPProxyKey))
}
+// AptNoProxy returns the 'apt-no proxy' for the environment.
@jameinel

jameinel Mar 23, 2017

Owner

typo 'apt-no-proxy'

Member

wupeka commented Mar 26, 2017

$$merge$$

Contributor

jujubot commented Mar 26, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 8b50ec0 into juju:develop Mar 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment