launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #33192
[Merge] ~enriqueesanchz/launchpad:add-return-import-vulnerability-job into launchpad:master
Enrique Sánchez has proposed merging ~enriqueesanchz/launchpad:add-return-import-vulnerability-job into launchpad:master.
Commit message:
Add VulnerabilityJobDerived.info property
- Use that property in `getImports`, `getExports` and job creation, to
return job info.
- Refactor test_vulnerability job tests
- Remove InformationType title conversion
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~enriqueesanchz/launchpad/+git/launchpad/+merge/494994
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~enriqueesanchz/launchpad:add-return-import-vulnerability-job into launchpad:master.
diff --git a/lib/lp/bugs/interfaces/vulnerabilityjob.py b/lib/lp/bugs/interfaces/vulnerabilityjob.py
index f057825..85b3b81 100644
--- a/lib/lp/bugs/interfaces/vulnerabilityjob.py
+++ b/lib/lp/bugs/interfaces/vulnerabilityjob.py
@@ -75,6 +75,12 @@ class IVulnerabilityJob(Interface):
def destroySelf():
"""Destroy this object."""
+ def info(self) -> dict:
+ """Retrieve information about the job.
+
+ :return: A dict of information about the job.
+ """
+
class VulnerabilityJobInProgress(Exception):
"""The VulnerabilityJob for the handler is already in progress."""
@@ -104,7 +110,7 @@ class IImportVulnerabilityJobSource(IJobSource):
def get(handler):
"""Retrieve the import job for a handler, if any.
- :return: `None` or an `IImportVulnerabilityJobSource`.
+ :return: `None` or an `IImportVulnerabilityJob`.
"""
@@ -134,7 +140,7 @@ class IExportVulnerabilityJobSource(IJobSource):
def get(handler):
"""Retrieve the export job for a handler, if any.
- :return: `None` or an `IExportVulnerabilityJobSource`.
+ :return: `None` or an `IExportVulnerabilityJob`.
"""
diff --git a/lib/lp/bugs/model/exportvulnerabilityjob.py b/lib/lp/bugs/model/exportvulnerabilityjob.py
index 6da2147..077850a 100644
--- a/lib/lp/bugs/model/exportvulnerabilityjob.py
+++ b/lib/lp/bugs/model/exportvulnerabilityjob.py
@@ -111,8 +111,6 @@ class ExportVulnerabilityJob(VulnerabilityJobDerived):
"error_description": [],
"succeeded": [],
"failed": [],
- },
- "data": {
"export_link": "",
},
}
@@ -345,7 +343,7 @@ class ExportVulnerabilityJob(VulnerabilityJobDerived):
expires=expires,
)
- self.metadata["data"]["export_link"] = lfa.getURL()
+ self.metadata["result"]["export_link"] = lfa.getURL()
def notifyUserError(self, error):
"""Calls up and also saves the error text in this job's metadata.
diff --git a/lib/lp/bugs/model/tests/test_vulnerability.py b/lib/lp/bugs/model/tests/test_vulnerability.py
index 30f49bc..91015e5 100644
--- a/lib/lp/bugs/model/tests/test_vulnerability.py
+++ b/lib/lp/bugs/model/tests/test_vulnerability.py
@@ -7,12 +7,12 @@ from pathlib import Path
from fixtures import MockPatch
from storm.store import Store
+from testscenarios.testcase import WithScenarios
from testtools.matchers import MatchesStructure
from zope.component import getUtility
from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
-from testscenarios.testcase import WithScenarios
from lp.app.enums import InformationType
from lp.app.errors import (
IncompatibleArguments,
@@ -678,6 +678,27 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
self.information_type = InformationType.PROPRIETARY
self.team = self.factory.makeTeam(members=[self.requester])
+ def _check_job_attributes(self, job, info, repo, expected_commit_sha1):
+ """Helper to check the common attributes of a created job."""
+ self.assertIsInstance(job, ImportVulnerabilityJob)
+ self.assertEqual(job.git_repository, removeSecurityProxy(repo).id)
+ self.assertEqual(job.git_ref, self.git_ref)
+ self.assertEqual(job.git_paths, self.git_paths)
+ self.assertEqual(job.information_type, self.information_type.value)
+ self.assertEqual(job.import_since_commit_sha1, expected_commit_sha1)
+
+ self.assertEqual(
+ info,
+ {
+ "job_details": {
+ "created_at": job.job.date_created,
+ "started_at": job.job.date_started,
+ "finished_at": job.job.date_finished,
+ },
+ **job.metadata,
+ },
+ )
+
def test_importData(self):
"""Test that we can create a ImportVulnerabilityJob using importData
method.
@@ -699,7 +720,7 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
)
with person_logged_in(self.requester):
- getUtility(IVulnerabilitySet).importData(
+ info = getUtility(IVulnerabilitySet).importData(
self.requester,
self.handler,
repo,
@@ -709,16 +730,11 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
import_since_commit_sha1=None,
)
- job = getUtility(IImportVulnerabilityJobSource).get(self.handler)
- naked_job = removeSecurityProxy(job)
- self.assertIsInstance(naked_job, ImportVulnerabilityJob)
- self.assertEqual(naked_job.git_repository, repo.id)
- self.assertEqual(naked_job.git_ref, self.git_ref)
- self.assertEqual(naked_job.git_paths, self.git_paths)
- self.assertEqual(
- naked_job.information_type, self.information_type.value
- )
- self.assertEqual(naked_job.import_since_commit_sha1, None)
+ job = getUtility(IImportVulnerabilityJobSource).get(self.handler)
+ naked_job = removeSecurityProxy(job)
+ self._check_job_attributes(
+ naked_job, info, repo, expected_commit_sha1=None
+ )
def test_importData_uct_handler(self):
"""Test that we can create a ImportVulnerabilityJob using importData
@@ -743,7 +759,7 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
handler = VulnerabilityHandlerEnum.UCT
with person_logged_in(self.requester):
- getUtility(IVulnerabilitySet).importData(
+ info = getUtility(IVulnerabilitySet).importData(
requester,
handler,
repo,
@@ -753,16 +769,11 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
import_since_commit_sha1=None,
)
- job = getUtility(IImportVulnerabilityJobSource).get(handler)
- naked_job = removeSecurityProxy(job)
- self.assertIsInstance(naked_job, ImportVulnerabilityJob)
- self.assertEqual(naked_job.git_repository, repo.id)
- self.assertEqual(naked_job.git_ref, self.git_ref)
- self.assertEqual(naked_job.git_paths, self.git_paths)
- self.assertEqual(
- naked_job.information_type, self.information_type.value
- )
- self.assertEqual(naked_job.import_since_commit_sha1, None)
+ job = getUtility(IImportVulnerabilityJobSource).get(handler)
+ naked_job = removeSecurityProxy(job)
+ self._check_job_attributes(
+ naked_job, info, repo, expected_commit_sha1=None
+ )
def test_importData_multiple_handlers(self):
"""Test that the feature flag allows to create ImportVulnerabilityJob
@@ -802,10 +813,10 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
import_since_commit_sha1=None,
)
- job = getUtility(IImportVulnerabilityJobSource).get(handler)
- naked_job = removeSecurityProxy(job)
- self.assertIsInstance(naked_job, ImportVulnerabilityJob)
- self.assertEqual(naked_job.handler, handler)
+ job = getUtility(IImportVulnerabilityJobSource).get(handler)
+ naked_job = removeSecurityProxy(job)
+ self.assertIsInstance(naked_job, ImportVulnerabilityJob)
+ self.assertEqual(naked_job.handler, handler)
def test_importData_feature_flag_disabled(self):
"""Test that if the feature flag is disabled it raises the
@@ -981,7 +992,7 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
import_since_commit_sha1 = "1" * 40
with person_logged_in(self.requester):
- getUtility(IVulnerabilitySet).importData(
+ info = getUtility(IVulnerabilitySet).importData(
self.requester,
self.handler,
repo,
@@ -991,18 +1002,14 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
import_since_commit_sha1,
)
- job = getUtility(IImportVulnerabilityJobSource).get(self.handler)
- naked_job = removeSecurityProxy(job)
- self.assertIsInstance(naked_job, ImportVulnerabilityJob)
- self.assertEqual(naked_job.git_repository, repo.id)
- self.assertEqual(naked_job.git_ref, self.git_ref)
- self.assertEqual(naked_job.git_paths, self.git_paths)
- self.assertEqual(
- naked_job.information_type, self.information_type.value
- )
- self.assertEqual(
- naked_job.import_since_commit_sha1, import_since_commit_sha1
- )
+ job = getUtility(IImportVulnerabilityJobSource).get(self.handler)
+ naked_job = removeSecurityProxy(job)
+ self._check_job_attributes(
+ naked_job,
+ info,
+ repo,
+ expected_commit_sha1=import_since_commit_sha1,
+ )
def test_importData_duplicated(self):
"""Test that we cannot create a duplicated ImportVulnerabilityJob
@@ -1165,7 +1172,7 @@ class TestVulnerabilitySetExportData(TestCaseWithFactory):
self.factory.makeDistribution(name="soss", owner=self.requester)
with person_logged_in(self.requester):
- getUtility(IVulnerabilitySet).exportData(
+ info = getUtility(IVulnerabilitySet).exportData(
self.requester,
self.handler,
None,
@@ -1175,6 +1182,17 @@ class TestVulnerabilitySetExportData(TestCaseWithFactory):
naked_job = removeSecurityProxy(job)
self.assertIsInstance(naked_job, ExportVulnerabilityJob)
self.assertEqual(naked_job.handler, self.handler)
+ self.assertEqual(
+ {
+ "job_details": {
+ "created_at": naked_job.job.date_created,
+ "started_at": naked_job.job.date_started,
+ "finished_at": naked_job.job.date_finished,
+ },
+ **naked_job.metadata,
+ },
+ info,
+ )
def test_exportData_multiple_handlers(self):
"""Test that the feature flag allows to create ExportVulnerabilityJob
@@ -1620,23 +1638,28 @@ class TestVulnerabilityGetJobs(WithScenarios, TestCaseWithFactory):
"""Tests for collecting export vulnerability job info."""
layer = LaunchpadZopelessLayer
-
+
scenarios = [
- ("soss", {
- "handler": VulnerabilityHandlerEnum.SOSS,
- "name": "soss",
- "distribution_name": "soss",
- "cve_key": "2025-1979",
- }),
- ("uct", {
- "handler": VulnerabilityHandlerEnum.UCT,
- "name": "uct",
- "distribution_name": "ubuntu",
- "cve_key": "2025-0001",
- })
+ (
+ "soss",
+ {
+ "handler": VulnerabilityHandlerEnum.SOSS,
+ "name": "soss",
+ "distribution_name": "soss",
+ "cve_key": "2025-1979",
+ },
+ ),
+ (
+ "uct",
+ {
+ "handler": VulnerabilityHandlerEnum.UCT,
+ "name": "uct",
+ "distribution_name": "ubuntu",
+ "cve_key": "2025-0001",
+ },
+ ),
]
-
def setUp(self):
super().setUp()
self.requester = self.factory.makePerson()
@@ -1646,10 +1669,12 @@ class TestVulnerabilityGetJobs(WithScenarios, TestCaseWithFactory):
repository=self.repository,
paths=("ref/heads/main", "ref/tags/v1.0"),
)
-
+
if self.name == "uct":
- self.distribution = getUtility(IDistributionSet).getByName(self.distribution_name)
-
+ self.distribution = getUtility(IDistributionSet).getByName(
+ self.distribution_name
+ )
+
# Create data from UCT CVE file
distroseries = self.factory.makeDistroSeries(
name="xenial",
@@ -1666,13 +1691,13 @@ class TestVulnerabilityGetJobs(WithScenarios, TestCaseWithFactory):
self.distribution = self.factory.makeDistribution(
name=self.distribution_name,
)
-
- # Grant permissions to the user that will request the data
+
+ # Grant permissions to the user that will request the data
self.distribution.security_admin = self.requester
def _create_import_job(self):
"""Helper to create a single ImportVulnerabilityJob."""
-
+
cve_name = "CVE-" + self.cve_key
cve_path = (
Path(__file__).parent
@@ -1684,7 +1709,7 @@ class TestVulnerabilityGetJobs(WithScenarios, TestCaseWithFactory):
/ "sampledata"
/ cve_name
)
-
+
with open(cve_path, "rb") as file:
self.useFixture(
GitHostingFixture(
@@ -1828,7 +1853,7 @@ class TestVulnerabilityGetJobs(WithScenarios, TestCaseWithFactory):
def test_getImports_unauthenticated(self):
"""Test imports fail when requester lacks handler rights."""
self.factory.makeCVE(self.cve_key)
-
+
# Remove permissions
self.distribution.security_admin = self.factory.makePerson()
@@ -1864,7 +1889,6 @@ class TestVulnerabilityGetJobs(WithScenarios, TestCaseWithFactory):
self.assertEqual(result["request"], naked_job.metadata["request"])
self.assertEqual(result["result"], naked_job.metadata["result"])
- self.assertEqual(result["data"], naked_job.metadata["data"])
job_details = {
"created_at": naked_job.job.date_created,
@@ -1900,7 +1924,6 @@ class TestVulnerabilityGetJobs(WithScenarios, TestCaseWithFactory):
self.assertEqual(result["request"], naked_job.metadata["request"])
self.assertEqual(result["result"], naked_job.metadata["result"])
- self.assertEqual(result["data"], naked_job.metadata["data"])
job_details = {
"created_at": naked_job.job.date_created,
@@ -1939,7 +1962,6 @@ class TestVulnerabilityGetJobs(WithScenarios, TestCaseWithFactory):
self.assertEqual(result["request"], naked_job.metadata["request"])
self.assertEqual(result["result"], naked_job.metadata["result"])
- self.assertEqual(result["data"], naked_job.metadata["data"])
job_details = {
"created_at": naked_job.job.date_created,
diff --git a/lib/lp/bugs/model/vulnerability.py b/lib/lp/bugs/model/vulnerability.py
index 81807c3..f19247b 100644
--- a/lib/lp/bugs/model/vulnerability.py
+++ b/lib/lp/bugs/model/vulnerability.py
@@ -481,7 +481,7 @@ class VulnerabilitySet:
# Trigger the import job after validations pass
job_source = getUtility(IImportVulnerabilityJobSource)
- job_source.create(
+ job = job_source.create(
handler,
git_repository.id,
git_ref,
@@ -490,7 +490,7 @@ class VulnerabilitySet:
import_since_commit_sha1,
)
- return None
+ return job.info
def exportData(
self,
@@ -533,39 +533,17 @@ class VulnerabilitySet:
# Trigger the export job after validations pass
job_source = getUtility(IExportVulnerabilityJobSource)
- job_source.create(handler)
+ job = job_source.create(handler)
- return None
+ return job.info
def _get_jobs_info(self, vulnerability_jobs):
# Merge fields from Job and VulnerabilityJob.metadata
+ from lp.bugs.model.vulnerabilityjob import VulnerabilityJobDerived
+
jobs_info = []
for vulnerability_job in vulnerability_jobs:
- job = vulnerability_job.job
- if not job:
- continue
-
- job_details = {
- "created_at": job.date_created,
- "started_at": job.date_started,
- "finished_at": job.date_finished,
- }
- job_info = dict(
- vulnerability_job.metadata or {}
- ).copy() # copy so we don’t mutate the original
-
- request = job_info.get("request")
- if request and "information_type" in request:
- info_type_number = request["information_type"]
- try:
- request["information_type"] = InformationType.items[
- info_type_number
- ].title
- except (KeyError, IndexError):
- request["information_type"] = ""
-
- job_info["job_details"] = job_details
- jobs_info.append(job_info)
+ jobs_info.append(VulnerabilityJobDerived(vulnerability_job).info)
return jobs_info
diff --git a/lib/lp/bugs/model/vulnerabilityjob.py b/lib/lp/bugs/model/vulnerabilityjob.py
index 80a0753..0a88be4 100644
--- a/lib/lp/bugs/model/vulnerabilityjob.py
+++ b/lib/lp/bugs/model/vulnerabilityjob.py
@@ -100,3 +100,14 @@ class VulnerabilityJobDerived(BaseRunnableJob, metaclass=EnumeratedSubclass):
]
)
return vars
+
+ @property
+ def info(self) -> dict:
+ return {
+ "job_details": {
+ "created_at": self.job.date_created,
+ "started_at": self.job.date_started,
+ "finished_at": self.job.date_finished,
+ },
+ **self.metadata,
+ }
diff --git a/lib/lp/bugs/tests/test_exportvulnerabilityjob.py b/lib/lp/bugs/tests/test_exportvulnerabilityjob.py
index 8408a40..fb099a6 100644
--- a/lib/lp/bugs/tests/test_exportvulnerabilityjob.py
+++ b/lib/lp/bugs/tests/test_exportvulnerabilityjob.py
@@ -148,8 +148,6 @@ class ExportVulnerabilityJobTests(WithScenarios, TestCaseWithFactory):
"error_description": [],
"succeeded": [],
"failed": [],
- },
- "data": {
"export_link": "",
},
}
@@ -238,8 +236,6 @@ class ExportVulnerabilityJobTests(WithScenarios, TestCaseWithFactory):
"error_description": [],
"succeeded": [],
"failed": [],
- },
- "data": {
"export_link": "",
},
}
@@ -327,8 +323,6 @@ class ExportVulnerabilityJobTests(WithScenarios, TestCaseWithFactory):
"error_description": [],
"succeeded": cve_names,
"failed": [],
- },
- "data": {
"export_link": export_link,
},
},
@@ -526,7 +520,7 @@ class ExportVulnerabilityTestViaCelery(WithScenarios, TestCaseWithFactory):
self.assertEqual(metadata_result, naked_job_metadata["result"])
self.assertThat(
- naked_job_metadata["data"]["export_link"],
+ naked_job_metadata["result"]["export_link"],
MatchesRegex(
r".*exported_vulnerabilities_[0-9]+\.zip$",
),
Follow ups
