launchpad-reviewers team mailing list archive

[Ines Almeida <mp+493139@xxxxxxxxxxxxxxxxxx>]

Left a couple of minor comments, thanks for this change!

Diff comments:

> diff --git a/lib/lp/bugs/model/tests/test_vulnerability.py b/lib/lp/bugs/model/tests/test_vulnerability.py
> index 5d7492e..631d104 100644
> --- a/lib/lp/bugs/model/tests/test_vulnerability.py
> +++ b/lib/lp/bugs/model/tests/test_vulnerability.py
> @@ -720,7 +723,9 @@ class TestVulnerabilitySetImportData(TestCaseWithFactory):
>          method and the UCT handler.
>          """
>          self.useContext(feature_flags())
> -        set_feature_flag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG, "true")
> +        set_feature_flag(
> +            VULNERABILITY_IMPORT_HANDLER_ENABLED_FEATURE_FLAG, "UCT Handler"

See comment below, I think this should be "UCT" only, right?

> +        )
>  
>          repo = self.factory.makeGitRepository(
>              owner=self.team, information_type=InformationType.USERDATA
> diff --git a/lib/lp/bugs/model/vulnerability.py b/lib/lp/bugs/model/vulnerability.py
> index 11f5dde..6503b8b 100644
> --- a/lib/lp/bugs/model/vulnerability.py
> +++ b/lib/lp/bugs/model/vulnerability.py
> @@ -485,9 +495,16 @@ class VulnerabilitySet:
>      ):
>          """See `IVulnerabilitySet`."""
>  
> -        if not getFeatureFlag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG):
> +        feature_flag = (
> +            getFeatureFlag(VULNERABILITY_EXPORT_HANDLER_ENABLED_FEATURE_FLAG)
> +            or ""
> +        )
> +
> +        enabled_handlers = [i.strip() for i in feature_flag.split()]
> +        if handler.name not in enabled_handlers:

Is handler.name "SOSS" or "SOSS Handler"?
I see both cases in the tests above. I'm guessing it would be "SOSS". It it had a space, we would have an issue when defining it in the feature flag since we separate by space

>              raise NotImplementedError(
> -                "Vulnerability export API is currently disabled"
> +                f"Vulnerability export API is currently disabled for "
> +                f"{handler.name}"

Nit, but I'd print the handler text ("SOSS Handler") instead of the name ("SOSS"), assuming that's what name prints. This is just because that's what users import in the handler
Another nit, I'd add '' around the handler to clarify that it's a value

>              )
>  
>          # Check requester's permissions to handler
> diff --git a/lib/lp/services/features/flags.py b/lib/lp/services/features/flags.py
> index c4472d8..da9d12b 100644
> --- a/lib/lp/services/features/flags.py
> +++ b/lib/lp/services/features/flags.py
> @@ -331,10 +331,19 @@ flag_info = sorted(
>              "",
>          ),
>          (
> -            "bugs.vulnerability_import_export.enabled",
> +            "bugs.vulnerability_import_handler.enabled",
>              "boolean",
>              "If true, users with the right permissions can trigger "

This should be changed in the line above to be "space delimited" (see jobs.celery.enabled_classes in this file for example)

Since this is not a "if true" anymore, perhaps it should say something more like "Lists handlers that users with the right permissions can use to trigger vulnerability data imports via the API" or similar

> -            "vulnerability data imports and exports via API",
> +            "vulnerability data imports for this handler via API",
> +            "",
> +            "",
> +            "",
> +        ),
> +        (
> +            "bugs.vulnerability_export_handler.enabled",
> +            "boolean",
> +            "If true, users with the right permissions can trigger "
> +            "vulnerability data exports for this handler via API",
>              "",
>              "",
>              "",


-- 
https://code.launchpad.net/~enriqueesanchz/launchpad/+git/launchpad/+merge/493139
Your team Launchpad code reviewers is requested to review the proposed merge of ~enriqueesanchz/launchpad:add-svt-handler-feature-flags into launchpad:master.