launchpad-reviewers team mailing list archive

[Enrique Sánchez <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:

does it make sense to change this to `isinstance(dict, vulnerability.metadata)` so we ensure we are able to use the `get()` method?

In theory, we will always have it as `None` or `dict`, but it might be a good practice.

> +            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:

same as above ^

Also, if "extra_attrs" is none we will be setting `metadata["extra_attrs"] = None`

> +            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

Why do we need this? We are storing extra_attrs in a list that will have the same order that the record has.

> +            # 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,

I don't see where you are changing this `bugtask_reference`

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

Is this linting change?

> +            },
>          )
>  
>          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.