Merge ~jfguedez/charm-juju-lint:bug/1943093 into charm-juju-lint:master

[Jose Guedez]

Remove requirement on pypi repositories for installation

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision f676a63d16d77e6f82e1e6e42e5e9ec18d51308d

diff --git a/lib/lib_jujulint.py b/lib/lib_jujulint.py
index 69a20fb..bce0fb4 100644
--- a/lib/lib_jujulint.py
+++ b/lib/lib_jujulint.py
@@ -15,7 +15,6 @@ from charmhelpers.contrib.charmsupport.nrpe import NRPE
 from charmhelpers.core import hookenv
 from charmhelpers.core.host import rsync
 from charmhelpers.fetch import snap
-from charmhelpers.fetch import apt_install
 from ops.model import (
     ActiveStatus,
     MaintenanceStatus,
@@ -33,7 +32,6 @@ class Paths:
     AUTO_LINT_SCRIPT_PATH = USR_LOCAL / "bin/auto_lint.py"
     AUTO_LINT_CRONTAB_PATH = pathlib.Path("/etc/cron.d/juju-lint")
     NAGIOS_PLUGINS_DIR = USR_LOCAL / "lib/nagios/plugins/"
-    VENV_DIR = pathlib.Path("/opt/juju_venvs/jujulint")
 
 
 CHECK_SHORTNAME = "juju_lint_results"
@@ -82,7 +80,21 @@ class JujuLintHelper:
         """
         charm_dir = pathlib.Path(hookenv.charm_dir())
         scripts_dir = charm_dir / "scripts"
-        rsync(str(scripts_dir / "auto_lint.py"), str(Paths.AUTO_LINT_SCRIPT_PATH))
+        templates_dir = scripts_dir / "templates"
+
+        # Create the auto_lint.py script from the template with the right permissions
+        with open(str(templates_dir / "auto_lint.py")) as f:
+            auto_lint_template = f.read()
+
+        auto_lint_script = auto_lint_template.replace(
+            "REPLACE_CHARMDIR", str(charm_dir)
+        )
+
+        fd = os.open(str(Paths.AUTO_LINT_SCRIPT_PATH), os.O_CREAT | os.O_WRONLY, 0o755)
+
+        with open(fd, "w") as f:
+            f.write(auto_lint_script)
+
         # install all files in scripts/plugins/ into nagios plugins dir
         plugins_dir = scripts_dir / "plugins"
         # Note that we need to add 'options' here to remove the flag '--delete' that
@@ -133,57 +145,6 @@ class JujuLintHelper:
         with Paths.AUTO_LINT_CONFIG_PATH.open("w") as fp:
             json.dump(dict(self.charm_config), fp, indent=4)  # cast from ConfigData
 
-    def create_venv(self):
-        """Create a virtual environment for the auto-lint script."""
-        apt_install(["python3-venv"])
-        subprocess.check_call(["python3", "-m", "venv", str(Paths.VENV_DIR)])
-        self._pip_install_venv("pip", upgrade=True)
-        self._pip_install_venv("juju", "pyyaml", upgrade=True)
-
-    @staticmethod
-    def _pip_install_venv(*pkgs, upgrade=False):
-        """Install packages in a virtual environment."""
-        # This charm originally used charmhelpers for this. However the ops
-        # framework runs the hooks with a modified PYTHONPATH already, which
-        # includes pyyaml. This caused the pip installation to skip pyyaml as
-        # it's considered already installed
-        # see the following bug reports for details:
-        #   * https://bugs.launchpad.net/charm-juju-lint/+bug/1928337
-        #   * https://github.com/juju/charm-helpers/issues/607
-        command = [str(Paths.VENV_DIR / "bin/pip"), "install"]
-
-        if upgrade:
-            command.append("--upgrade")
-
-        command.extend(pkgs)
-        logging.debug("installing packages using: {}".format(command))
-
-        # drop PYTHONPATH if present
-        env = os.environ.copy()
-        if "PYTHONPATH" in env:
-            env.pop("PYTHONPATH")
-
-        # handle juju special proxy environment variables: JUJU_CHARM_*_PROXY
-        # this is not handled in charmhelpers, see the following resources
-        # for details and overall references on the issue:
-        #   *  https://discourse.charmhub.io/t/command-model-config/1768
-        #   *  https://github.com/juju/charm-helpers/issues/614
-        #   *  https://github.com/juju/charm-helpers/pull/248/
-        #   *  https://bugs.launchpad.net/charm-octavia-diskimage-retrofit/+bug/1843510
-        proxy_envvars = hookenv.env_proxy_settings()
-        logging.debug("proxy_envvars: {}".format(proxy_envvars))
-
-        if proxy_envvars is not None:
-            env.update(proxy_envvars)
-
-        try:
-            pip_output = subprocess.check_output(command, env=env).decode("utf8")
-        except subprocess.CalledProcessError as error:
-            logging.error("pip install failed:\n{}".format(error.output.decode("utf8")))
-            raise error
-
-        logging.debug("pip install succeeded:\n{}".format(pip_output))
-
     @staticmethod
     def run_auto_lint():
         """Run auto lint."""
diff --git a/requirements.txt b/requirements.txt
index b12b148..6093c4e 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,2 +1,3 @@
 charmhelpers
+juju
 ops
diff --git a/scripts/auto_lint.py b/scripts/templates/auto_lint.py
similarity index 93%
rename from scripts/auto_lint.py
rename to scripts/templates/auto_lint.py
index cbc4389..9476682 100755
--- a/scripts/auto_lint.py
+++ b/scripts/templates/auto_lint.py
@@ -1,4 +1,4 @@
-#!/opt/juju_venvs/jujulint/bin/python
+#!/usr/bin/env python3
 
 """
 Automatically grab `juju status` output and analyse it with Juju Lint.
@@ -26,11 +26,14 @@ import logging
 from os.path import join
 import os
 import subprocess
-from sys import exit
+import sys
 
-from juju import loop
-from juju.model import Model
-from juju.client.facade import TypeEncoder
+# The path below is templated in during charm install
+sys.path.append("REPLACE_CHARMDIR/venv")
+
+from juju import loop  # noqa E402
+from juju.model import Model  # noqa E402
+from juju.client.facade import TypeEncoder  # noqa E402
 
 VAR_LIB = "/var/snap/juju-lint/common"
 LINT_CONFIG_PATH = join(VAR_LIB, "auto-lint-config.json")
@@ -65,7 +68,7 @@ def verify_auto_lint_config(auto_lint_config):
     )
     for option in required:
         if not auto_lint_config[option]:
-            exit("Juju Lint: Please set model connection config values.")
+            sys.exit("Juju Lint: Please set model connection config values.")
 
 
 async def get_juju_status(auto_lint_config):
@@ -250,7 +253,7 @@ def main():
     pid = str(os.getpid())
 
     if os.path.isfile(PID_FILENAME):
-        exit("{} already exists, exiting".format(PID_FILENAME))
+        sys.exit("{} already exists, exiting".format(PID_FILENAME))
 
     with open(PID_FILENAME, "w") as f:
         f.write(pid)
diff --git a/src/charm.py b/src/charm.py
index cc6b77e..2b8b543 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -48,7 +48,6 @@ class JujuLintCharm(CharmBase):
     def on_install(self, event):
         """Install charm and perform initial configuration."""
         self.helper.install_snap(event)
-        self.helper.create_venv()
         self.helper.create_install_dirs()
         self.helper.deploy_scripts()
         self.helper.update_config()
diff --git a/tests/unit/test_auto_lint.py b/tests/unit/test_auto_lint.py
index 00189ae..edbed7c 100644
--- a/tests/unit/test_auto_lint.py
+++ b/tests/unit/test_auto_lint.py
@@ -4,7 +4,7 @@ import unittest
 
 import mock
 from juju import loop
-from scripts.auto_lint import (
+from scripts.templates.auto_lint import (
     add_charm_config_to_juju_status,
     lint_juju,
     process_juju_status,
@@ -117,8 +117,8 @@ class TestAutoLint(unittest.TestCase):
 
         self.assertRaises(KeyError, test_helper)
 
-    @mock.patch("scripts.auto_lint.write_file")
-    @mock.patch("scripts.auto_lint.subprocess.check_output")
+    @mock.patch("scripts.templates.auto_lint.write_file")
+    @mock.patch("scripts.templates.auto_lint.subprocess.check_output")
     def test_nrpe_split_json(self, mock_check_output, mock_write_file):
         """Check that juju-lint outputs json when running in split checks mode."""
         auto_lint_config = {
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index aff54f8..2eec635 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -5,12 +5,13 @@ import pathlib
 import tempfile
 import unittest
 
-from ops.testing import Harness
+from charmhelpers.core.hookenv import charm_dir
 import mock
+from ops.testing import Harness
 import yaml
 
 from src.charm import JujuLintCharm
-from lib_jujulint import Paths, JujuLintHelper
+from lib_jujulint import Paths
 
 
 def reparent(pathsobj, name):
@@ -90,10 +91,14 @@ class TestCharm(unittest.TestCase):
         self.assertFalse(self.harness.charm.state.installed)
 
     @mock.patch("charmhelpers.fetch.snap.snap_install")
+    @mock.patch("charmhelpers.core.hookenv.charm_dir", wraps=charm_dir)
     @mock.patch("lib_jujulint.subprocess")
-    @mock.patch("lib_jujulint.apt_install")
-    def test_05_install(self, mock_apt_install, mock_subprocess, mock_snap_install):
+    def test_05_install(self, mock_subprocess, mock_charm_dir, mock_snap_install):
         """Test: charm install."""
+        tmp = pathlib.Path(self.tmpdir.name)
+        charm_dir_abs = os.path.abspath(charm_dir())
+        mock_charm_dir.return_value = charm_dir_abs
+
         self.harness.begin()
         self.harness.charm.on.install.emit()
         self.assertEqual(self.harness.charm.unit.status.name, "active")
@@ -107,11 +112,21 @@ class TestCharm(unittest.TestCase):
             "usr/local/lib/nagios/plugins/check_juju_lint_results.py",
             "var/snap/juju-lint/common/auto-lint-config.json",
         ]
-        tmp = pathlib.Path(self.tmpdir.name)
+
         for expect in expected_files:
             p = tmp / expect
             self.assertTrue(p.is_file())
 
+        # Test that auto_lint.py was templated correctly
+        with open(str(tmp / "usr/local/bin/auto_lint.py")) as f:
+            auto_lint_script = f.read()
+
+        self.assertIn(
+            "{}/venv".format(charm_dir_abs),
+            auto_lint_script,
+        )
+        self.assertNotIn("REPLACE_CHARMDIR", auto_lint_script)
+
     def test_09_config_changed_defer(self):
         """Test: config changed event is deferred if charm not installed."""
         self.harness.begin()
@@ -230,60 +245,6 @@ class TestCharm(unittest.TestCase):
             {"juju_lint_results-misc", "juju_lint_results-config"},
         )
 
-    @mock.patch("lib_jujulint.os")
-    @mock.patch("lib_jujulint.subprocess")
-    def test_25_pip_install_env(self, mock_subprocess, mock_os):
-        """Test: pip install function."""
-        # make sure the environment has a PYTHONPATH variable
-        mock_os.environ.copy.return_value = {"PYTHONPATH": "lib:venv"}
-
-        # get the full path to pip within the tempdir for the test
-        tmp_pip_path = str(pathlib.Path(self.tmpdir.name) / Paths.VENV_DIR / "bin/pip")
-
-        # generate the expected inputs to subprocess.check_output
-        expected_command = [tmp_pip_path, "install", "pyyaml"]
-        expected_env = {}
-
-        # invoke the package install and validate the inputs
-        JujuLintHelper._pip_install_venv("pyyaml")
-        mock_subprocess.check_output.assert_called_with(
-            expected_command, env=expected_env
-        )
-
-        # test the upgrade flag
-        expected_command = [tmp_pip_path, "install", "--upgrade", "pip"]
-        JujuLintHelper._pip_install_venv("pip", upgrade=True)
-        mock_subprocess.check_output.assert_called_with(
-            expected_command, env=expected_env
-        )
-
-    @mock.patch("lib_jujulint.os")
-    @mock.patch("lib_jujulint.subprocess")
-    @mock.patch("charmhelpers.core.hookenv.env_proxy_settings")
-    def test_26_pip_install_proxy_vars(
-        self, mock_env_proxy_settings, mock_subprocess, mock_os
-    ):
-        """Test: handling of special juju proxy variables."""
-        mock_os.environ.copy.return_value = {
-            "HTTPS_PROXY": "https://invalid.proxy:3128"
-        }
-        mock_env_proxy_settings.return_value = {
-            "HTTPS_PROXY": "https://squid.internals:3128"
-        }
-
-        # get the full path to pip within the tempdir for the test
-        tmp_pip_path = str(pathlib.Path(self.tmpdir.name) / Paths.VENV_DIR / "bin/pip")
-
-        # generate the expected inputs to subprocess.check_output
-        expected_command = [tmp_pip_path, "install", "pyyaml"]
-        expected_env = {"HTTPS_PROXY": "https://squid.internals:3128"}
-
-        # invoke the package install and validate the inputs
-        JujuLintHelper._pip_install_venv("pyyaml")
-        mock_subprocess.check_output.assert_called_with(
-            expected_command, env=expected_env
-        )
-
     def get_notice_count(self, hook):
         """Return the notice count for a given charm hook."""
         notice_count = 0