launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
> index e4edcae..64f4e9c 100644
> --- a/lib/lp/oci/model/ocirecipe.py
> +++ b/lib/lp/oci/model/ocirecipe.py
> @@ -141,6 +144,10 @@ class OCIRecipe(Storm, WebhookTargetMixin):
>      owner_id = Int(name='owner', allow_none=False)
>      owner = Reference(owner_id, 'Person.id')
>  
> +    _information_type = DBEnum(
> +        enum=InformationType, default=InformationType.PUBLIC,
> +        name="information_type")

I feel like this is going to need a validator for the same sorts of reasons that `Snap._information_type` has one: public recipes shouldn't be allowed to have private Git repositories or private owners.  (We may not particularly need a feature flag in this case, though.)

> +
>      oci_project_id = Int(name='oci_project', allow_none=False)
>      oci_project = Reference(oci_project_id, "OCIProject.id")
>  
> @@ -198,6 +206,23 @@ class OCIRecipe(Storm, WebhookTargetMixin):
>              self.oci_project.name, self.name)
>  
>      @property
> +    def information_type(self):
> +        if self._information_type is None:
> +            return InformationType.PUBLIC
> +        return self._information_type
> +
> +    @information_type.setter
> +    def information_type(self, information_type):
> +        if information_type == self._information_type:
> +            return
> +        self._information_type = information_type
> +        self._reconcileAccess()
> +
> +    @property
> +    def pillar(self):
> +        return self.oci_project.pillar

Should this be mentioned somewhere in the interface?

> +
> +    @property
>      def valid_webhook_event_types(self):
>          return ["oci-recipe:build:0.1"]
>  
> diff --git a/lib/lp/registry/browser/pillar.py b/lib/lp/registry/browser/pillar.py
> index 52b1554..c5bfd3f 100644
> --- a/lib/lp/registry/browser/pillar.py
> +++ b/lib/lp/registry/browser/pillar.py
> @@ -439,15 +439,22 @@ class PillarPersonSharingView(LaunchpadView):
>      def _loadSharedArtifacts(self):
>          # As a concrete can by linked via more than one policy, we use sets to
>          # filter out dupes.
> -        (self.bugtasks, self.branches, self.gitrepositories, self.snaps,
> -         self.specifications) = (
> -            self.sharing_service.getSharedArtifacts(
> -                self.pillar, self.person, self.user))
> +        artifacts = self.sharing_service.getSharedArtifacts(
> +                self.pillar, self.person, self.user)
> +        self.bugtasks = artifacts["bugtasks"]
> +        self.branches = artifacts["branches"]
> +        self.gitrepositories = artifacts["gitrepositories"]
> +        self.snaps = artifacts["snaps"]
> +        self.specifications = artifacts["specifications"]
> +        self.ocirecipes = artifacts["ocirecipes"]
> +
>          bug_ids = set([bugtask.bug.id for bugtask in self.bugtasks])
>          self.shared_bugs_count = len(bug_ids)
>          self.shared_branches_count = len(self.branches)
>          self.shared_gitrepositories_count = len(self.gitrepositories)
> +        self.shared_snaps_count = len(self.snaps)
>          self.shared_specifications_count = len(self.specifications)
> +        self.shared_ocirecipe_count = len(self.ocirecipes)

These are only used by lib/lp/registry/templates/pillar-sharing-details.pt, so perhaps it's worth updating that bit of UI while you're here?  (Or in a separate branch if you prefer.)

>  
>      def _build_specification_template_data(self, specs, request):
>          spec_data = []
> diff --git a/lib/lp/registry/interfaces/sharingservice.py b/lib/lp/registry/interfaces/sharingservice.py
> index f6c39fb..b000090 100644
> --- a/lib/lp/registry/interfaces/sharingservice.py
> +++ b/lib/lp/registry/interfaces/sharingservice.py
> @@ -335,11 +354,14 @@ class ISharingService(IService):
>              title=_('Snap recipes'), required=False),
>          specifications=List(
>              Reference(schema=ISpecification), title=_('Specifications'),
> -            required=False))
> +            required=False),
> +        ocirecipes=List(
> +            Reference(schema=IOCIRecipe),
> +            title=_('Snap recipes'), required=False))

s/Snap/OCI/

>      @operation_for_version('devel')
>      def revokeAccessGrants(pillar, grantee, user, bugs=None, branches=None,
>                             gitrepositories=None, snaps=None,
> -                           specifications=None):
> +                           specifications=None, ocirecipes=None):
>          """Remove a grantee's access to the specified artifacts.
>  
>          :param pillar: the pillar from which to remove access
> @@ -350,6 +372,7 @@ class ISharingService(IService):
>          :param gitrepositories: the Git repositories for which to revoke access
>          :param snaps: The snap recipes for which to revoke access
>          :param specifications: the specifications for which to revoke access
> +        :param snaps: The OCI recipes for which to revoke access

s/:param snaps:/:param ocirecipes:/, I think.

>          """
>  
>      @export_write_operation()
> @@ -367,12 +390,15 @@ class ISharingService(IService):
>              title=_('Git repositories'), required=False),
>          snaps=List(
>              Reference(schema=ISnap),
> -            title=_('Snap recipes'), required=False)
> +            title=_('Snap recipes'), required=False),
> +        ocirecipes=List(
> +            Reference(schema=ISnap),

s/ISnap/IOCIRecipe/

> +            title=_('OCI recipes'), required=False)
>      )
>      @operation_for_version('devel')
>      def ensureAccessGrants(grantees, user, bugs=None, branches=None,
>                             gitrepositories=None, snaps=None,
> -                           specifications=None):
> +                           specifications=None, ocirecipes=None):
>          """Ensure a grantee has an access grant to the specified artifacts.
>  
>          :param grantees: the people or teams for whom to grant access
> diff --git a/lib/lp/registry/services/sharingservice.py b/lib/lp/registry/services/sharingservice.py
> index 4909604..5fc99c8 100644
> --- a/lib/lp/registry/services/sharingservice.py
> +++ b/lib/lp/registry/services/sharingservice.py
> @@ -245,48 +254,68 @@ class SharingService:
>          specifications = []
>          if specification_ids:
>              specifications = load(Specification, specification_ids)
> +        ocirecipes = []
> +        if ocirecipe_ids:
> +            ocirecipes = load(OCIRecipe, ocirecipe_ids)
>  
> -        return bugtasks, branches, gitrepositories, snaps, specifications
> +        return {"bugtasks": bugtasks, "branches": branches,
> +                "gitrepositories": gitrepositories, "snaps": snaps,
> +                "specifications": specifications, "ocirecipes": ocirecipes}

Yay!

>  
>      @available_with_permission('launchpad.Driver', 'pillar')
>      def getSharedBugs(self, pillar, person, user):
>          """See `ISharingService`."""
> -        bugtasks, _, _, _, _ = self.getSharedArtifacts(
> +        artifacts = self.getSharedArtifacts(
>              pillar, person, user, include_branches=False,
> -            include_gitrepositories=False, include_specifications=False)
> -        return bugtasks
> +            include_gitrepositories=False,
> +            include_specifications=False, include_snaps=False,
> +            include_ocirecipes=False)
> +        return artifacts["bugtasks"]
>  
>      @available_with_permission('launchpad.Driver', 'pillar')
>      def getSharedBranches(self, pillar, person, user):
>          """See `ISharingService`."""
> -        _, branches, _, _, _ = self.getSharedArtifacts(
> +        artifacts = self.getSharedArtifacts(
>              pillar, person, user, include_bugs=False,
> -            include_gitrepositories=False, include_specifications=False)
> -        return branches
> +            include_gitrepositories=False, include_specifications=False,
> +            include_snaps=False, include_ocirecipes=False)
> +        return artifacts["branches"]
>  
>      @available_with_permission('launchpad.Driver', 'pillar')
>      def getSharedGitRepositories(self, pillar, person, user):
>          """See `ISharingService`."""
> -        _, _, gitrepositories, _, _ = self.getSharedArtifacts(
> +        artifacts = self.getSharedArtifacts(
>              pillar, person, user, include_bugs=False, include_branches=False,
> -            include_specifications=False)
> -        return gitrepositories
> +            include_specifications=False, include_snaps=False,
> +            include_ocirecipes=False)
> +        return artifacts["gitrepositories"]
>  
>      @available_with_permission('launchpad.Driver', 'pillar')
>      def getSharedSnaps(self, pillar, person, user):
>          """See `ISharingService`."""
> -        _, _, _, snaps, _ = self.getSharedArtifacts(
> +        artifacts = self.getSharedArtifacts(
>              pillar, person, user, include_bugs=False, include_branches=False,
> -            include_gitrepositories=False)
> -        return snaps
> +            include_gitrepositories=False, include_specifications=False,
> +            include_ocirecipes=False)
> +        return artifacts["snaps"]
>  
>      @available_with_permission('launchpad.Driver', 'pillar')
>      def getSharedSpecifications(self, pillar, person, user):
>          """See `ISharingService`."""
> -        _, _, _, _, specifications = self.getSharedArtifacts(
> +        artifacts = self.getSharedArtifacts(
>              pillar, person, user, include_bugs=False, include_branches=False,
> -            include_gitrepositories=False)
> -        return specifications
> +            include_gitrepositories=False, include_snaps=False,
> +            include_ocirecipes=False)
> +        return artifacts["specifications"]
> +
> +    @available_with_permission('launchpad.Driver', 'pillar')
> +    def getSharedOCIRecipes(self, pillar, person, user):
> +        """See `ISharingService`."""
> +        artifacts = self.getSharedArtifacts(
> +            pillar, person, user, include_bugs=False, include_branches=False,
> +            include_gitrepositories=False, include_snaps=False,
> +            include_specifications=False)
> +        return artifacts["ocirecipes"]
>  
>      def _getVisiblePrivateSpecificationIDs(self, person, specifications):
>          store = Store.of(specifications[0])
> @@ -809,6 +841,18 @@ class SharingService:
>          getUtility(IAccessArtifactGrantSource).revokeByArtifact(
>              artifacts_to_delete, [grantee])
>  
> +        # XXX: Pappacena 2021-02-05: snaps should not trigger this job,
> +        # since we do not have a "SnapSubscription" yet.
> +        artifacts = [i for i in artifacts if not ISnap.providedBy(i)]
> +        if not artifacts:
> +            return

You've already landed `SnapSubscription` on master, so should probably delete this bit.

> +
> +        # XXX: Pappacena 2021-03-09: OCI recipes should not trigger this job,
> +        # since we do not have a "OCIRecipeSubscription" yet.
> +        artifacts = [i for i in artifacts if not IOCIRecipe.providedBy(i)]
> +        if not artifacts:
> +            return
> +
>          # Create a job to remove subscriptions for artifacts the grantee can no
>          # longer see.
>          return getUtility(IRemoveArtifactSubscriptionsJobSource).create(


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/399394
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:ocirecipe-private-accesspolicy.