From 1ab33b64166b05ca4d51c9e7b8e6a231ac58bdd9 Mon Sep 17 00:00:00 2001 From: "Brian C. Lane" Date: Wed, 30 May 2018 15:13:02 -0700 Subject: [PATCH] Fix handling bad source repos and add a test When adding a source failed it wasn't being removed from the dnf object. This fixes that, and returns an error when setting up the source fails. Also adds a test for it. This also includes detecting rawhide vs. non-rawhide releases and adjusting the tests accordingly (some of the source names change). (cherry picked from commit dd8e4d9e9906d5762cd825086fcd51d0c2ed1111) --- src/pylorax/api/projects.py | 4 ++++ src/pylorax/api/v0.py | 18 +++++++++++++----- tests/pylorax/test_server.py | 32 ++++++++++++++++++++++++++++---- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/pylorax/api/projects.py b/src/pylorax/api/projects.py index c5206bda..48e928de 100644 --- a/src/pylorax/api/projects.py +++ b/src/pylorax/api/projects.py @@ -405,6 +405,10 @@ def source_to_repo(source, dnf_conf): """ repo = dnf.repo.Repo(source["name"], dnf_conf) + # This will allow errors to be raised so we can catch them + # without this they are logged, but the repo is silently disabled + repo.skip_if_unavailable = False + if source["type"] == "yum-baseurl": repo.baseurl = [source["url"]] elif source["type"] == "yum-metalink": diff --git a/src/pylorax/api/v0.py b/src/pylorax/api/v0.py index 08e6b402..89d9bd17 100644 --- a/src/pylorax/api/v0.py +++ b/src/pylorax/api/v0.py @@ -1451,11 +1451,6 @@ def v0_api(api): if source["name"] in repos: del dbo.repos[source["name"]] - # XXX - BCL DIAGNOSTIC - repos = list(r.id for r in dbo.repos.iter_enabled()) - if source["name"] in repos: - return jsonify(status=False, errors=["Failed to delete DNF repo %s" % source["name"]]), 400 - repo = source_to_repo(source, dbo.conf) dbo.repos.add(repo) @@ -1477,6 +1472,19 @@ def v0_api(api): with open(source_path, "w") as f: f.write(str(repo)) except Exception as e: + log.error("(v0_projects_source_add) adding %s failed: %s", source["name"], str(e)) + + # Cleanup the mess, if loading it failed we don't want to leave it in memory + repos = list(r.id for r in dbo.repos.iter_enabled()) + if source["name"] in repos: + with api.config["DNFLOCK"].lock: + dbo = api.config["DNFLOCK"].dbo + del dbo.repos[source["name"]] + + log.info("Updating repository metadata after adding %s failed", source["name"]) + dbo.fill_sack(load_system_repo=False) + dbo.read_comps() + return jsonify(status=False, errors=[str(e)]), 400 return jsonify(status=True) diff --git a/tests/pylorax/test_server.py b/tests/pylorax/test_server.py index c50b2387..61061d4a 100644 --- a/tests/pylorax/test_server.py +++ b/tests/pylorax/test_server.py @@ -35,6 +35,7 @@ class ServerTestCase(unittest.TestCase): @classmethod def setUpClass(self): + self.rawhide = False self.maxDiff = None repo_dir = tempfile.mkdtemp(prefix="lorax.test.repo.") @@ -55,6 +56,10 @@ class ServerTestCase(unittest.TestCase): for f in glob("./tests/pylorax/repos/*.repo"): shutil.copy2(f, dnf_repo_dir) + # Modify fedora vs. rawhide tests when running on rawhide + if os.path.exists("/etc/yum.repos.d/fedora-rawhide.repo"): + self.rawhide = True + # dnf repo baseurl has to point to an absolute directory, so we use /tmp/lorax-empty-repo/ in the files # and create an empty repository os.makedirs("/tmp/lorax-empty-repo/") @@ -486,7 +491,10 @@ class ServerTestCase(unittest.TestCase): resp = self.server.get("/api/v0/projects/source/list") data = json.loads(resp.data) self.assertNotEqual(data, None) - self.assertEqual(data["sources"], ["lorax-1", "lorax-2", "lorax-3", "lorax-4", "other-repo", "rawhide", "single-repo"]) + if self.rawhide: + self.assertEqual(data["sources"], ["lorax-1", "lorax-2", "lorax-3", "lorax-4", "other-repo", "rawhide", "single-repo"]) + else: + self.assertEqual(data["sources"], ["fedora", "lorax-1", "lorax-2", "lorax-3", "lorax-4", "other-repo", "single-repo", "updates"]) def test_projects_source_00_info(self): """Test /api/v0/projects/source/info""" @@ -550,18 +558,34 @@ class ServerTestCase(unittest.TestCase): self.assertEqual(repo["check_ssl"], False) self.assertTrue("gpgkey_urls" not in repo) + def test_projects_source_00_bad_url(self): + """Test /api/v0/projects/source/new with a new source that has an invalid url""" + toml_source = open("./tests/pylorax/source/bad-repo.toml").read() + self.assertTrue(len(toml_source) > 0) + resp = self.server.post("/api/v0/projects/source/new", + data=toml_source, + content_type="text/x-toml") + data = json.loads(resp.data) + self.assertEqual(data["status"], False) + def test_projects_source_01_delete_system(self): """Test /api/v0/projects/source/delete a system source""" - resp = self.server.delete("/api/v0/projects/source/delete/rawhide") + if self.rawhide: + resp = self.server.delete("/api/v0/projects/source/delete/rawhide") + else: + resp = self.server.delete("/api/v0/projects/source/delete/fedora") data = json.loads(resp.data) self.assertNotEqual(data, None) self.assertEqual(data["status"], False) - # Make sure rawhide is still listed + # Make sure fedora/rawhide is still listed resp = self.server.get("/api/v0/projects/source/list") data = json.loads(resp.data) self.assertNotEqual(data, None) - self.assertTrue("rawhide" in data["sources"]) + if self.rawhide: + self.assertTrue("rawhide" in data["sources"]) + else: + self.assertTrue("fedora" in data["sources"]) def test_projects_source_02_delete_single(self): """Test /api/v0/projects/source/delete a single source"""