Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try JUJU_CHARM_*_PROXY settings when fetching jenkins deb #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dannf
Copy link
Contributor

@dannf dannf commented Feb 21, 2019

If our model has juju proxy settings and we've been told to
fetch a specific deb, set the corresponding *_proxy variables
when doing so. If the user has set JUJU HTTP(S)/FTP proxies,
but has pointed us to an internally hosted deb, hopefully
they've also set JUJU_CHARM_NO_PROXY appropriately. But,
just in case, leave the fallback of fetching w an unmodified
environment.

Copy link
Collaborator

@freeekanayaka freeekanayaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are failing tests, I also have a small nit.

@dannf
Copy link
Contributor Author

dannf commented Feb 21, 2019

Oh - but still failing tests. I'll look into that.

@dannf
Copy link
Contributor Author

dannf commented Feb 21, 2019

I fixed one of the coverage tests. For the other, I need to mock a non-zero wget exit code, but I'm not sure how to do that. @freeekanayaka - do you have any advice there?

@freeekanayaka
Copy link
Collaborator

@dannf looking

@freeekanayaka
Copy link
Collaborator

@dannf to be able to write a test we'd need to teach the fake wget to use a different URL if it finds an "http_proxy" variable in the environment (the environment should be available in the proc_args dict that the Wget.call method receive).

Then you could write a test like:

    def test_install_jenkins_remote(self):
        """
        If the 'release' config is set to a remote URL, and the JUJU_CHARM proxy environment
        variables are set, then the charm first tries to download the deb using the proxy and falls
        back to no proxy in case of failures.
        """
        self.useFixture(EnvironmentVariable("JUJU_CHARM_HTTP_PROXY", "1.2.3.4"))
        self.fakes.processes.wget.locations[
            "http://jenkins-1.2.3.deb"] = b"data"
        orig_release = hookenv.config()["release"]
        try:
            hookenv.config()["release"] = "http://jenkins-1.2.3.deb"
            self.packages.install_jenkins()
            self.assertEqual(
                ["install"], self.fakes.processes.dpkg.actions["jenkins"])
        finally:
            hookenv.config()["release"] = orig_release

which would hit the new code path that you added in this branch.

It should be relatively easy to modify Wget, but for now I think we can just add a "no cover" directive for this new code, e.g.:

except subprocess.CalledProcessError: # pragma: no cover
            if len(proxy_env) == 0:
                raise
            subprocess.check_call(("wget", "-q", "-O", target, url))

I can file a bug against systemfixtures to track this.

If our model has juju proxy settings and we've been told to
fetch a specific deb, set the corresponding *_proxy variables
when doing so. If the user has set JUJU HTTP(S)/FTP proxies,
but has pointed us to an internally hosted deb, hopefully
they've also set JUJU_CHARM_NO_PROXY appropriately. But,
just in case, leave the fallback of fetching w an unmodified
environment.

Skip coverage test for wget failures until we have the
necessary fixture support. See:
 testing-cabal/systemfixtures#2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants