launchpad-reviewers team mailing list archive

[İlker Emre Koç <mp+493278@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/bugs/scripts/soss/sossexport.py b/lib/lp/bugs/scripts/soss/sossexport.py
> index 94a7f98..6256146 100644
> --- a/lib/lp/bugs/scripts/soss/sossexport.py
> +++ b/lib/lp/bugs/scripts/soss/sossexport.py
> @@ -104,6 +104,11 @@ class SOSSExporter(SVTExporter):
>              bug.bugtasks[0].assignee.name if bug.bugtasks[0].assignee else ""
>          )
>  
> +        if vulnerability.metadata:

Makes sense ^^. Will do!

> +            extra_attrs = vulnerability.metadata.get("extra_attrs")
> +        else:
> +            extra_attrs = None
> +
>          # Parse vulnerability
>          public_date = self._normalize_date_without_timezone(
>              vulnerability.date_made_public
> diff --git a/lib/lp/bugs/scripts/soss/sossimport.py b/lib/lp/bugs/scripts/soss/sossimport.py
> index 2bb7c17..6017204 100644
> --- a/lib/lp/bugs/scripts/soss/sossimport.py
> +++ b/lib/lp/bugs/scripts/soss/sossimport.py
> @@ -288,6 +294,11 @@ class SOSSImporter(SVTImporter):
>          vulnerability.date_coordinated_release = None
>          vulnerability.cvss = self._prepare_cvss_data(soss_record)
>  
> +        if vulnerability.metadata:

Hmm good point, maybe it will be better if we do not.

> +            vulnerability.metadata["extra_attrs"] = soss_record.extra_attrs
> +        elif soss_record.extra_attrs:
> +            vulnerability.metadata = {"extra_attrs": soss_record.extra_attrs}
> +
>          logger.info(
>              "[SOSSImporter] Updated Vulnerability with ID: "
>              f"{vulnerability.id} for {vulnerability.distribution.name}",
> diff --git a/lib/lp/bugs/scripts/soss/tests/test_sossexport.py b/lib/lp/bugs/scripts/soss/tests/test_sossexport.py
> index 1752d8e..26b72a6 100644
> --- a/lib/lp/bugs/scripts/soss/tests/test_sossexport.py
> +++ b/lib/lp/bugs/scripts/soss/tests/test_sossexport.py
> @@ -90,6 +90,29 @@ class TestSOSSExporter(TestCaseWithFactory):
>              self.soss, information_type=InformationType.PROPRIETARY
>          )
>  
> +        original_to_dict = SOSSRecord.to_dict
> +
> +        def mock_to_dict(*args, **kwargs):
> +            # Ensure that Extra_attrs is always sorted to avoid test failures

Extra_attrs is a dictionary no? It is written as such in the specs.

And we need to use this for this test in particular because the to_yaml function kept writing the dictionary into the file in a different order. Causing the test to fail even though the contents are identical.

> +            # due to dictionary order differences.
> +
> +            result = original_to_dict(*args, **kwargs)
> +
> +            if "Extra_attrs" in result:
> +                result["Extra_attrs"] = dict(
> +                    sorted(
> +                        result["Extra_attrs"].items(), key=lambda item: item[0]
> +                    )
> +                )
> +
> +            return result
> +
> +        self.patch(
> +            SOSSRecord,
> +            "to_dict",
> +            mock_to_dict,
> +        )
> +
>          for file in self.sampledata.iterdir():
>  
>              bug, vulnerability = soss_importer.import_cve_from_file(file)
> diff --git a/lib/lp/bugs/scripts/soss/tests/test_sossimport.py b/lib/lp/bugs/scripts/soss/tests/test_sossimport.py
> index 675da82..6efc532 100644
> --- a/lib/lp/bugs/scripts/soss/tests/test_sossimport.py
> +++ b/lib/lp/bugs/scripts/soss/tests/test_sossimport.py
> @@ -197,7 +204,7 @@ class TestSOSSImporter(TestCaseWithFactory):
>  
>          self._check_bugtasks(
>              bug.bugtasks,
> -            self.bugtask_reference,
> +            bugtask_reference,

Ah this was basically a bug-fix. This particular function had a `bugtask_reference` parameter that was unusued and the only usecase of this function was putting `self.bugtask_reference` into that field. And I assumed the self.bugtask_reference would be typo because of this as it didn't make sense to be there.

>              BugTaskImportance.LOW,
>              self.janitor,
>          )
> @@ -378,7 +387,9 @@ class TestSOSSImporter(TestCaseWithFactory):
>              ),
>              BugTaskStatus.DEFERRED,
>              "test note",
> -            {"repositories": ["test-repo"]},
> +            {
> +                "repositories": ["test-repo"],

Ah, I made a change to that part and later deleted it, but didn't fix the positoning and it seems linter didn't see a problem with it. Will return it to its original state.

> +            },
>          )
>  
>          soss_importer._create_or_update_bugtasks(bug, self.soss_record)


-- 
https://code.launchpad.net/~ilkeremrekoc/launchpad/+git/launchpad/+merge/493278
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilkeremrekoc/launchpad:add-extra-attrs into launchpad:master.