Merge ~jfguedez/charm-juju-lint:bug/1943093 into charm-juju-lint:master
- Git
- lp:~jfguedez/charm-juju-lint
- bug/1943093
- Merge into master
Proposed by
Jose Guedez
Status: | Merged |
---|---|
Approved by: | James Troup |
Approved revision: | 387bf2c9133c82d80ea450bc14960e1c948ba8bf |
Merged at revision: | f676a63d16d77e6f82e1e6e42e5e9ec18d51308d |
Proposed branch: | ~jfguedez/charm-juju-lint:bug/1943093 |
Merge into: | charm-juju-lint:master |
Diff against target: |
316 lines (+49/-124) 6 files modified
lib/lib_jujulint.py (+15/-54) requirements.txt (+1/-0) scripts/templates/auto_lint.py (+10/-7) src/charm.py (+0/-1) tests/unit/test_auto_lint.py (+3/-3) tests/unit/test_charm.py (+20/-59) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
BootStack Reviewers | Pending | ||
BootStack Reviewers | Pending | ||
Review via email: mp+408341@code.launchpad.net |
Commit message
Remove requirement on pypi repositories for installation
Description of the change
To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Change successfully merged at revision f676a63d16d77e6
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lib/lib_jujulint.py b/lib/lib_jujulint.py | |||
2 | index 69a20fb..bce0fb4 100644 | |||
3 | --- a/lib/lib_jujulint.py | |||
4 | +++ b/lib/lib_jujulint.py | |||
5 | @@ -15,7 +15,6 @@ from charmhelpers.contrib.charmsupport.nrpe import NRPE | |||
6 | 15 | from charmhelpers.core import hookenv | 15 | from charmhelpers.core import hookenv |
7 | 16 | from charmhelpers.core.host import rsync | 16 | from charmhelpers.core.host import rsync |
8 | 17 | from charmhelpers.fetch import snap | 17 | from charmhelpers.fetch import snap |
9 | 18 | from charmhelpers.fetch import apt_install | ||
10 | 19 | from ops.model import ( | 18 | from ops.model import ( |
11 | 20 | ActiveStatus, | 19 | ActiveStatus, |
12 | 21 | MaintenanceStatus, | 20 | MaintenanceStatus, |
13 | @@ -33,7 +32,6 @@ class Paths: | |||
14 | 33 | AUTO_LINT_SCRIPT_PATH = USR_LOCAL / "bin/auto_lint.py" | 32 | AUTO_LINT_SCRIPT_PATH = USR_LOCAL / "bin/auto_lint.py" |
15 | 34 | AUTO_LINT_CRONTAB_PATH = pathlib.Path("/etc/cron.d/juju-lint") | 33 | AUTO_LINT_CRONTAB_PATH = pathlib.Path("/etc/cron.d/juju-lint") |
16 | 35 | NAGIOS_PLUGINS_DIR = USR_LOCAL / "lib/nagios/plugins/" | 34 | NAGIOS_PLUGINS_DIR = USR_LOCAL / "lib/nagios/plugins/" |
17 | 36 | VENV_DIR = pathlib.Path("/opt/juju_venvs/jujulint") | ||
18 | 37 | 35 | ||
19 | 38 | 36 | ||
20 | 39 | CHECK_SHORTNAME = "juju_lint_results" | 37 | CHECK_SHORTNAME = "juju_lint_results" |
21 | @@ -82,7 +80,21 @@ class JujuLintHelper: | |||
22 | 82 | """ | 80 | """ |
23 | 83 | charm_dir = pathlib.Path(hookenv.charm_dir()) | 81 | charm_dir = pathlib.Path(hookenv.charm_dir()) |
24 | 84 | scripts_dir = charm_dir / "scripts" | 82 | scripts_dir = charm_dir / "scripts" |
26 | 85 | rsync(str(scripts_dir / "auto_lint.py"), str(Paths.AUTO_LINT_SCRIPT_PATH)) | 83 | templates_dir = scripts_dir / "templates" |
27 | 84 | |||
28 | 85 | # Create the auto_lint.py script from the template with the right permissions | ||
29 | 86 | with open(str(templates_dir / "auto_lint.py")) as f: | ||
30 | 87 | auto_lint_template = f.read() | ||
31 | 88 | |||
32 | 89 | auto_lint_script = auto_lint_template.replace( | ||
33 | 90 | "REPLACE_CHARMDIR", str(charm_dir) | ||
34 | 91 | ) | ||
35 | 92 | |||
36 | 93 | fd = os.open(str(Paths.AUTO_LINT_SCRIPT_PATH), os.O_CREAT | os.O_WRONLY, 0o755) | ||
37 | 94 | |||
38 | 95 | with open(fd, "w") as f: | ||
39 | 96 | f.write(auto_lint_script) | ||
40 | 97 | |||
41 | 86 | # install all files in scripts/plugins/ into nagios plugins dir | 98 | # install all files in scripts/plugins/ into nagios plugins dir |
42 | 87 | plugins_dir = scripts_dir / "plugins" | 99 | plugins_dir = scripts_dir / "plugins" |
43 | 88 | # Note that we need to add 'options' here to remove the flag '--delete' that | 100 | # Note that we need to add 'options' here to remove the flag '--delete' that |
44 | @@ -133,57 +145,6 @@ class JujuLintHelper: | |||
45 | 133 | with Paths.AUTO_LINT_CONFIG_PATH.open("w") as fp: | 145 | with Paths.AUTO_LINT_CONFIG_PATH.open("w") as fp: |
46 | 134 | json.dump(dict(self.charm_config), fp, indent=4) # cast from ConfigData | 146 | json.dump(dict(self.charm_config), fp, indent=4) # cast from ConfigData |
47 | 135 | 147 | ||
48 | 136 | def create_venv(self): | ||
49 | 137 | """Create a virtual environment for the auto-lint script.""" | ||
50 | 138 | apt_install(["python3-venv"]) | ||
51 | 139 | subprocess.check_call(["python3", "-m", "venv", str(Paths.VENV_DIR)]) | ||
52 | 140 | self._pip_install_venv("pip", upgrade=True) | ||
53 | 141 | self._pip_install_venv("juju", "pyyaml", upgrade=True) | ||
54 | 142 | |||
55 | 143 | @staticmethod | ||
56 | 144 | def _pip_install_venv(*pkgs, upgrade=False): | ||
57 | 145 | """Install packages in a virtual environment.""" | ||
58 | 146 | # This charm originally used charmhelpers for this. However the ops | ||
59 | 147 | # framework runs the hooks with a modified PYTHONPATH already, which | ||
60 | 148 | # includes pyyaml. This caused the pip installation to skip pyyaml as | ||
61 | 149 | # it's considered already installed | ||
62 | 150 | # see the following bug reports for details: | ||
63 | 151 | # * https://bugs.launchpad.net/charm-juju-lint/+bug/1928337 | ||
64 | 152 | # * https://github.com/juju/charm-helpers/issues/607 | ||
65 | 153 | command = [str(Paths.VENV_DIR / "bin/pip"), "install"] | ||
66 | 154 | |||
67 | 155 | if upgrade: | ||
68 | 156 | command.append("--upgrade") | ||
69 | 157 | |||
70 | 158 | command.extend(pkgs) | ||
71 | 159 | logging.debug("installing packages using: {}".format(command)) | ||
72 | 160 | |||
73 | 161 | # drop PYTHONPATH if present | ||
74 | 162 | env = os.environ.copy() | ||
75 | 163 | if "PYTHONPATH" in env: | ||
76 | 164 | env.pop("PYTHONPATH") | ||
77 | 165 | |||
78 | 166 | # handle juju special proxy environment variables: JUJU_CHARM_*_PROXY | ||
79 | 167 | # this is not handled in charmhelpers, see the following resources | ||
80 | 168 | # for details and overall references on the issue: | ||
81 | 169 | # * https://discourse.charmhub.io/t/command-model-config/1768 | ||
82 | 170 | # * https://github.com/juju/charm-helpers/issues/614 | ||
83 | 171 | # * https://github.com/juju/charm-helpers/pull/248/ | ||
84 | 172 | # * https://bugs.launchpad.net/charm-octavia-diskimage-retrofit/+bug/1843510 | ||
85 | 173 | proxy_envvars = hookenv.env_proxy_settings() | ||
86 | 174 | logging.debug("proxy_envvars: {}".format(proxy_envvars)) | ||
87 | 175 | |||
88 | 176 | if proxy_envvars is not None: | ||
89 | 177 | env.update(proxy_envvars) | ||
90 | 178 | |||
91 | 179 | try: | ||
92 | 180 | pip_output = subprocess.check_output(command, env=env).decode("utf8") | ||
93 | 181 | except subprocess.CalledProcessError as error: | ||
94 | 182 | logging.error("pip install failed:\n{}".format(error.output.decode("utf8"))) | ||
95 | 183 | raise error | ||
96 | 184 | |||
97 | 185 | logging.debug("pip install succeeded:\n{}".format(pip_output)) | ||
98 | 186 | |||
99 | 187 | @staticmethod | 148 | @staticmethod |
100 | 188 | def run_auto_lint(): | 149 | def run_auto_lint(): |
101 | 189 | """Run auto lint.""" | 150 | """Run auto lint.""" |
102 | diff --git a/requirements.txt b/requirements.txt | |||
103 | index b12b148..6093c4e 100644 | |||
104 | --- a/requirements.txt | |||
105 | +++ b/requirements.txt | |||
106 | @@ -1,2 +1,3 @@ | |||
107 | 1 | charmhelpers | 1 | charmhelpers |
108 | 2 | juju | ||
109 | 2 | ops | 3 | ops |
110 | diff --git a/scripts/auto_lint.py b/scripts/templates/auto_lint.py | |||
111 | 3 | similarity index 93% | 4 | similarity index 93% |
112 | 4 | rename from scripts/auto_lint.py | 5 | rename from scripts/auto_lint.py |
113 | 5 | rename to scripts/templates/auto_lint.py | 6 | rename to scripts/templates/auto_lint.py |
114 | index cbc4389..9476682 100755 | |||
115 | --- a/scripts/auto_lint.py | |||
116 | +++ b/scripts/templates/auto_lint.py | |||
117 | @@ -1,4 +1,4 @@ | |||
119 | 1 | #!/opt/juju_venvs/jujulint/bin/python | 1 | #!/usr/bin/env python3 |
120 | 2 | 2 | ||
121 | 3 | """ | 3 | """ |
122 | 4 | Automatically grab `juju status` output and analyse it with Juju Lint. | 4 | Automatically grab `juju status` output and analyse it with Juju Lint. |
123 | @@ -26,11 +26,14 @@ import logging | |||
124 | 26 | from os.path import join | 26 | from os.path import join |
125 | 27 | import os | 27 | import os |
126 | 28 | import subprocess | 28 | import subprocess |
128 | 29 | from sys import exit | 29 | import sys |
129 | 30 | 30 | ||
133 | 31 | from juju import loop | 31 | # The path below is templated in during charm install |
134 | 32 | from juju.model import Model | 32 | sys.path.append("REPLACE_CHARMDIR/venv") |
135 | 33 | from juju.client.facade import TypeEncoder | 33 | |
136 | 34 | from juju import loop # noqa E402 | ||
137 | 35 | from juju.model import Model # noqa E402 | ||
138 | 36 | from juju.client.facade import TypeEncoder # noqa E402 | ||
139 | 34 | 37 | ||
140 | 35 | VAR_LIB = "/var/snap/juju-lint/common" | 38 | VAR_LIB = "/var/snap/juju-lint/common" |
141 | 36 | LINT_CONFIG_PATH = join(VAR_LIB, "auto-lint-config.json") | 39 | LINT_CONFIG_PATH = join(VAR_LIB, "auto-lint-config.json") |
142 | @@ -65,7 +68,7 @@ def verify_auto_lint_config(auto_lint_config): | |||
143 | 65 | ) | 68 | ) |
144 | 66 | for option in required: | 69 | for option in required: |
145 | 67 | if not auto_lint_config[option]: | 70 | if not auto_lint_config[option]: |
147 | 68 | exit("Juju Lint: Please set model connection config values.") | 71 | sys.exit("Juju Lint: Please set model connection config values.") |
148 | 69 | 72 | ||
149 | 70 | 73 | ||
150 | 71 | async def get_juju_status(auto_lint_config): | 74 | async def get_juju_status(auto_lint_config): |
151 | @@ -250,7 +253,7 @@ def main(): | |||
152 | 250 | pid = str(os.getpid()) | 253 | pid = str(os.getpid()) |
153 | 251 | 254 | ||
154 | 252 | if os.path.isfile(PID_FILENAME): | 255 | if os.path.isfile(PID_FILENAME): |
156 | 253 | exit("{} already exists, exiting".format(PID_FILENAME)) | 256 | sys.exit("{} already exists, exiting".format(PID_FILENAME)) |
157 | 254 | 257 | ||
158 | 255 | with open(PID_FILENAME, "w") as f: | 258 | with open(PID_FILENAME, "w") as f: |
159 | 256 | f.write(pid) | 259 | f.write(pid) |
160 | diff --git a/src/charm.py b/src/charm.py | |||
161 | index cc6b77e..2b8b543 100755 | |||
162 | --- a/src/charm.py | |||
163 | +++ b/src/charm.py | |||
164 | @@ -48,7 +48,6 @@ class JujuLintCharm(CharmBase): | |||
165 | 48 | def on_install(self, event): | 48 | def on_install(self, event): |
166 | 49 | """Install charm and perform initial configuration.""" | 49 | """Install charm and perform initial configuration.""" |
167 | 50 | self.helper.install_snap(event) | 50 | self.helper.install_snap(event) |
168 | 51 | self.helper.create_venv() | ||
169 | 52 | self.helper.create_install_dirs() | 51 | self.helper.create_install_dirs() |
170 | 53 | self.helper.deploy_scripts() | 52 | self.helper.deploy_scripts() |
171 | 54 | self.helper.update_config() | 53 | self.helper.update_config() |
172 | diff --git a/tests/unit/test_auto_lint.py b/tests/unit/test_auto_lint.py | |||
173 | index 00189ae..edbed7c 100644 | |||
174 | --- a/tests/unit/test_auto_lint.py | |||
175 | +++ b/tests/unit/test_auto_lint.py | |||
176 | @@ -4,7 +4,7 @@ import unittest | |||
177 | 4 | 4 | ||
178 | 5 | import mock | 5 | import mock |
179 | 6 | from juju import loop | 6 | from juju import loop |
181 | 7 | from scripts.auto_lint import ( | 7 | from scripts.templates.auto_lint import ( |
182 | 8 | add_charm_config_to_juju_status, | 8 | add_charm_config_to_juju_status, |
183 | 9 | lint_juju, | 9 | lint_juju, |
184 | 10 | process_juju_status, | 10 | process_juju_status, |
185 | @@ -117,8 +117,8 @@ class TestAutoLint(unittest.TestCase): | |||
186 | 117 | 117 | ||
187 | 118 | self.assertRaises(KeyError, test_helper) | 118 | self.assertRaises(KeyError, test_helper) |
188 | 119 | 119 | ||
191 | 120 | @mock.patch("scripts.auto_lint.write_file") | 120 | @mock.patch("scripts.templates.auto_lint.write_file") |
192 | 121 | @mock.patch("scripts.auto_lint.subprocess.check_output") | 121 | @mock.patch("scripts.templates.auto_lint.subprocess.check_output") |
193 | 122 | def test_nrpe_split_json(self, mock_check_output, mock_write_file): | 122 | def test_nrpe_split_json(self, mock_check_output, mock_write_file): |
194 | 123 | """Check that juju-lint outputs json when running in split checks mode.""" | 123 | """Check that juju-lint outputs json when running in split checks mode.""" |
195 | 124 | auto_lint_config = { | 124 | auto_lint_config = { |
196 | diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py | |||
197 | index aff54f8..2eec635 100644 | |||
198 | --- a/tests/unit/test_charm.py | |||
199 | +++ b/tests/unit/test_charm.py | |||
200 | @@ -5,12 +5,13 @@ import pathlib | |||
201 | 5 | import tempfile | 5 | import tempfile |
202 | 6 | import unittest | 6 | import unittest |
203 | 7 | 7 | ||
205 | 8 | from ops.testing import Harness | 8 | from charmhelpers.core.hookenv import charm_dir |
206 | 9 | import mock | 9 | import mock |
207 | 10 | from ops.testing import Harness | ||
208 | 10 | import yaml | 11 | import yaml |
209 | 11 | 12 | ||
210 | 12 | from src.charm import JujuLintCharm | 13 | from src.charm import JujuLintCharm |
212 | 13 | from lib_jujulint import Paths, JujuLintHelper | 14 | from lib_jujulint import Paths |
213 | 14 | 15 | ||
214 | 15 | 16 | ||
215 | 16 | def reparent(pathsobj, name): | 17 | def reparent(pathsobj, name): |
216 | @@ -90,10 +91,14 @@ class TestCharm(unittest.TestCase): | |||
217 | 90 | self.assertFalse(self.harness.charm.state.installed) | 91 | self.assertFalse(self.harness.charm.state.installed) |
218 | 91 | 92 | ||
219 | 92 | @mock.patch("charmhelpers.fetch.snap.snap_install") | 93 | @mock.patch("charmhelpers.fetch.snap.snap_install") |
220 | 94 | @mock.patch("charmhelpers.core.hookenv.charm_dir", wraps=charm_dir) | ||
221 | 93 | @mock.patch("lib_jujulint.subprocess") | 95 | @mock.patch("lib_jujulint.subprocess") |
224 | 94 | @mock.patch("lib_jujulint.apt_install") | 96 | def test_05_install(self, mock_subprocess, mock_charm_dir, mock_snap_install): |
223 | 95 | def test_05_install(self, mock_apt_install, mock_subprocess, mock_snap_install): | ||
225 | 96 | """Test: charm install.""" | 97 | """Test: charm install.""" |
226 | 98 | tmp = pathlib.Path(self.tmpdir.name) | ||
227 | 99 | charm_dir_abs = os.path.abspath(charm_dir()) | ||
228 | 100 | mock_charm_dir.return_value = charm_dir_abs | ||
229 | 101 | |||
230 | 97 | self.harness.begin() | 102 | self.harness.begin() |
231 | 98 | self.harness.charm.on.install.emit() | 103 | self.harness.charm.on.install.emit() |
232 | 99 | self.assertEqual(self.harness.charm.unit.status.name, "active") | 104 | self.assertEqual(self.harness.charm.unit.status.name, "active") |
233 | @@ -107,11 +112,21 @@ class TestCharm(unittest.TestCase): | |||
234 | 107 | "usr/local/lib/nagios/plugins/check_juju_lint_results.py", | 112 | "usr/local/lib/nagios/plugins/check_juju_lint_results.py", |
235 | 108 | "var/snap/juju-lint/common/auto-lint-config.json", | 113 | "var/snap/juju-lint/common/auto-lint-config.json", |
236 | 109 | ] | 114 | ] |
238 | 110 | tmp = pathlib.Path(self.tmpdir.name) | 115 | |
239 | 111 | for expect in expected_files: | 116 | for expect in expected_files: |
240 | 112 | p = tmp / expect | 117 | p = tmp / expect |
241 | 113 | self.assertTrue(p.is_file()) | 118 | self.assertTrue(p.is_file()) |
242 | 114 | 119 | ||
243 | 120 | # Test that auto_lint.py was templated correctly | ||
244 | 121 | with open(str(tmp / "usr/local/bin/auto_lint.py")) as f: | ||
245 | 122 | auto_lint_script = f.read() | ||
246 | 123 | |||
247 | 124 | self.assertIn( | ||
248 | 125 | "{}/venv".format(charm_dir_abs), | ||
249 | 126 | auto_lint_script, | ||
250 | 127 | ) | ||
251 | 128 | self.assertNotIn("REPLACE_CHARMDIR", auto_lint_script) | ||
252 | 129 | |||
253 | 115 | def test_09_config_changed_defer(self): | 130 | def test_09_config_changed_defer(self): |
254 | 116 | """Test: config changed event is deferred if charm not installed.""" | 131 | """Test: config changed event is deferred if charm not installed.""" |
255 | 117 | self.harness.begin() | 132 | self.harness.begin() |
256 | @@ -230,60 +245,6 @@ class TestCharm(unittest.TestCase): | |||
257 | 230 | {"juju_lint_results-misc", "juju_lint_results-config"}, | 245 | {"juju_lint_results-misc", "juju_lint_results-config"}, |
258 | 231 | ) | 246 | ) |
259 | 232 | 247 | ||
260 | 233 | @mock.patch("lib_jujulint.os") | ||
261 | 234 | @mock.patch("lib_jujulint.subprocess") | ||
262 | 235 | def test_25_pip_install_env(self, mock_subprocess, mock_os): | ||
263 | 236 | """Test: pip install function.""" | ||
264 | 237 | # make sure the environment has a PYTHONPATH variable | ||
265 | 238 | mock_os.environ.copy.return_value = {"PYTHONPATH": "lib:venv"} | ||
266 | 239 | |||
267 | 240 | # get the full path to pip within the tempdir for the test | ||
268 | 241 | tmp_pip_path = str(pathlib.Path(self.tmpdir.name) / Paths.VENV_DIR / "bin/pip") | ||
269 | 242 | |||
270 | 243 | # generate the expected inputs to subprocess.check_output | ||
271 | 244 | expected_command = [tmp_pip_path, "install", "pyyaml"] | ||
272 | 245 | expected_env = {} | ||
273 | 246 | |||
274 | 247 | # invoke the package install and validate the inputs | ||
275 | 248 | JujuLintHelper._pip_install_venv("pyyaml") | ||
276 | 249 | mock_subprocess.check_output.assert_called_with( | ||
277 | 250 | expected_command, env=expected_env | ||
278 | 251 | ) | ||
279 | 252 | |||
280 | 253 | # test the upgrade flag | ||
281 | 254 | expected_command = [tmp_pip_path, "install", "--upgrade", "pip"] | ||
282 | 255 | JujuLintHelper._pip_install_venv("pip", upgrade=True) | ||
283 | 256 | mock_subprocess.check_output.assert_called_with( | ||
284 | 257 | expected_command, env=expected_env | ||
285 | 258 | ) | ||
286 | 259 | |||
287 | 260 | @mock.patch("lib_jujulint.os") | ||
288 | 261 | @mock.patch("lib_jujulint.subprocess") | ||
289 | 262 | @mock.patch("charmhelpers.core.hookenv.env_proxy_settings") | ||
290 | 263 | def test_26_pip_install_proxy_vars( | ||
291 | 264 | self, mock_env_proxy_settings, mock_subprocess, mock_os | ||
292 | 265 | ): | ||
293 | 266 | """Test: handling of special juju proxy variables.""" | ||
294 | 267 | mock_os.environ.copy.return_value = { | ||
295 | 268 | "HTTPS_PROXY": "https://invalid.proxy:3128" | ||
296 | 269 | } | ||
297 | 270 | mock_env_proxy_settings.return_value = { | ||
298 | 271 | "HTTPS_PROXY": "https://squid.internals:3128" | ||
299 | 272 | } | ||
300 | 273 | |||
301 | 274 | # get the full path to pip within the tempdir for the test | ||
302 | 275 | tmp_pip_path = str(pathlib.Path(self.tmpdir.name) / Paths.VENV_DIR / "bin/pip") | ||
303 | 276 | |||
304 | 277 | # generate the expected inputs to subprocess.check_output | ||
305 | 278 | expected_command = [tmp_pip_path, "install", "pyyaml"] | ||
306 | 279 | expected_env = {"HTTPS_PROXY": "https://squid.internals:3128"} | ||
307 | 280 | |||
308 | 281 | # invoke the package install and validate the inputs | ||
309 | 282 | JujuLintHelper._pip_install_venv("pyyaml") | ||
310 | 283 | mock_subprocess.check_output.assert_called_with( | ||
311 | 284 | expected_command, env=expected_env | ||
312 | 285 | ) | ||
313 | 286 | |||
314 | 287 | def get_notice_count(self, hook): | 248 | def get_notice_count(self, hook): |
315 | 288 | """Return the notice count for a given charm hook.""" | 249 | """Return the notice count for a given charm hook.""" |
316 | 289 | notice_count = 0 | 250 | notice_count = 0 |
This merge proposal is being monitored by mergebot. Change the status to Approved to merge.