launchpad-reviewers team mailing list archive

[Ines Almeida <mp+491584@xxxxxxxxxxxxxxxxxx>]

CI failed, but I think just a flaky run
Replied to the existing questions

Diff comments:

> diff --git a/lib/lp/bugs/interfaces/vulnerability.py b/lib/lp/bugs/interfaces/vulnerability.py
> index 90a9829..bda5c1a 100644
> --- a/lib/lp/bugs/interfaces/vulnerability.py
> +++ b/lib/lp/bugs/interfaces/vulnerability.py
> @@ -381,6 +394,94 @@ class IVulnerabilitySet(Interface):
>      def findByIds(vulnerability_ids, visible_by_user=None):
>          """Returns the vulnerabilities with the given IDs."""
>  
> +    @operation_parameters(
> +        handler=Choice(
> +            title=_("The handler to import or export vulnerability data"),
> +            readonly=True,
> +            required=True,
> +            vocabulary=VulnerabilityHandlerEnum,
> +        ),
> +        git_repository=Reference(schema=IGitRepository),
> +        git_ref=TextLine(
> +            title=_("The git reference to import from."),
> +            required=True,
> +        ),
> +        git_paths=List(
> +            title=_("The list of paths to import from."),
> +            value_type=TextLine(),
> +            required=True,
> +        ),
> +        information_type=copy_field(
> +            IVulnerabilityEditableAttributes["information_type"]
> +        ),
> +        import_since_commit_sha1=TextLine(
> +            title=_(
> +                "Commit sha1 from which to do the import. All files added or "
> +                "modified since this commit will be imported."
> +            ),
> +            required=False,
> +        ),
> +    )
> +    @call_with(requester=REQUEST_USER)
> +    @export_write_operation()
> +    @operation_for_version("devel")
> +    def importData(
> +        requester,
> +        handler,
> +        git_repository,
> +        git_ref,
> +        git_paths,
> +        information_type,
> +        import_since_commit_sha1,
> +    ):
> +        """Trigger a vulnerability data import
> +
> +        :param handler: The handler that will perform the import.
> +        :param git_repository: The git repository to import from.
> +        :param git_ref: The git reference to import from.
> +        :param git_paths: The paths in the git repo to import from.

Hmm, specially given that we for sure want this parameter for SOSS and UCT (because we really only want the '/active' changes, right?), it really won't make a huge difference for now, and I agree it makes it a lot more conscious.

Let's keep it (maybe add just a comment with `[""] will get everything`) and we'll evolve the functionality as we go - can even be picked up by now joiners as it should be quite simple

> +        :param information_type: The information type of the vulnerability.
> +            If the git repo is private, the information type must be PRIVATE.
> +        :param import_since_commit_sha1: The commit SHA1 to import from.
> +            If null, import all data.
> +        """
> +
> +    @operation_parameters(
> +        handler=Choice(
> +            title=_("The handler to import or export vulnerability data"),
> +            readonly=True,
> +            required=True,
> +            vocabulary=VulnerabilityHandlerEnum,
> +        ),
> +        sources=List(
> +            title=_(
> +                "List of objects to get data from. If null, import all data."
> +            ),
> +            required=False,
> +        ),
> +    )
> +    @call_with(requester=REQUEST_USER)
> +    @export_write_operation()
> +    @operation_for_version("devel")
> +    def exportData(
> +        requester,
> +        handler,
> +        sources,
> +    ):
> +        """Trigger a vulnerability data export
> +
> +        :param handler: The handler that will perform the export.
> +        :param sources: List of objects to get data from. If null, import all
> +            data.
> +        """
> +
> +    @collection_default_content()
> +    def empty_list():
> +        """Return an empty collection of vulnerabilities.
> +
> +        This only exists to keep lazr.restful happy.
> +        """
> +
>  
>  class IVulnerabilityActivity(Interface):
>      """`IVulnerabilityActivity` attributes that require launchpad.View."""
> diff --git a/lib/lp/bugs/model/vulnerability.py b/lib/lp/bugs/model/vulnerability.py
> index c33cc0f..4d4ba07 100644
> --- a/lib/lp/bugs/model/vulnerability.py
> +++ b/lib/lp/bugs/model/vulnerability.py
> @@ -382,6 +392,96 @@ class VulnerabilitySet:
>              clauses.append(get_vulnerability_privacy_filter(visible_by_user))
>          return IStore(Vulnerability).find(Vulnerability, *clauses)
>  
> +    def importData(
> +        self,
> +        requester,
> +        handler,
> +        git_repository,
> +        git_ref,
> +        git_paths,
> +        information_type,
> +        import_since_commit_sha1,
> +    ):
> +        """See `IVulnerabilitySet`."""
> +
> +        if not getFeatureFlag(VULNERABILITY_IMPORT_ENABLED_FEATURE_FLAG):
> +            raise NotImplementedError(
> +                "Vulnerability import API is currently disabled"
> +            )
> +
> +        # Launchpad's object permissions automatically check requester's
> +        # permissions to git repo
> +
> +        # Check requester's permissions to handler
> +        from lp.bugs.enums import VulnerabilityHandlerEnum
> +        from lp.bugs.scripts.soss.sossimport import SOSSImporter
> +
> +        if handler == VulnerabilityHandlerEnum.SOSS:
> +            importer = SOSSImporter()
> +        else:
> +            raise Exception("Handler not found")
> +
> +        if not importer.checkUserPermissions(requester):
> +            raise UnauthorizedVulnerabilityHandler(
> +                f"You don't have enough rights to use {handler}"
> +            )
> +
> +        # If repository is private, information_type must be PRIVATE
> +        if (
> +            git_repository.private
> +            and information_type in PUBLIC_INFORMATION_TYPES
> +        ):
> +            # TODO check what's usually the validation error codes we use

Hmmmmmm, it seems like when I try to check for `validate` functions most of them seem to raise specific errors or AssertionError. I also see a IncompatibleArguments, maybe could also work for this one in particular

For non existing git ref in a git repo etc I'd say it should be NotFoundError

> +            raise ValueError(
> +                "information_type must be PRIVATE when importing from a "
> +                "private git repository"
> +            )
> +
> +        repo_ref = git_repository.getRefByPath(git_ref)
> +        if repo_ref is None:
> +            raise ValueError(
> +                f"{git_ref} does not exist in the specified git repository"
> +            )
> +
> +        # Check import_since_commit_sha1 exists in git_ref
> +        if import_since_commit_sha1 and not git_repository.checkCommitInRef(
> +            import_since_commit_sha1, git_ref
> +        ):
> +            raise ValueError(
> +                f"{import_since_commit_sha1} does not exist in {git_ref}"
> +            )
> +
> +        # Trigger the import job after validations pass
> +        job_source = getUtility(IImportVulnerabilityJobSource)
> +        job_source.create(

Right, maybe the exception that it raises is already the right one.
Yeah, let's create a small ticket to return the in progress job details here, and leave this for now!

> +            handler,
> +            git_repository.git_https_url,
> +            git_ref,
> +            git_paths,
> +            information_type.value,
> +            import_since_commit_sha1,
> +        )
> +
> +        return None
> +
> +    def exportData(
> +        self,
> +        requester,
> +        handler,
> +        sources,
> +    ):
> +        """See `IVulnerabilitySet`."""
> +
> +        raise NotImplementedError(
> +            "Vulnerability export API is currently disabled"
> +        )
> +
> +        return None
> +
> +    def empty_list(self):
> +        """See `IVulnerabilitySet`."""
> +        return []
> +
>  
>  @implementer(IVulnerabilityActivity)
>  class VulnerabilityActivity(StormBase):


-- 
https://code.launchpad.net/~enriqueesanchz/launchpad/+git/launchpad/+merge/491584
Your team Launchpad code reviewers is requested to review the proposed merge of ~enriqueesanchz/launchpad:add-import-export-endpoints into launchpad:master.