launchpad-reviewers team mailing list archive

[Ines Almeida <mp+472800@xxxxxxxxxxxxxxxxxx>]

I think this is great work and seems like a huge beast to tackle.

I do feel like it misses an opportunity to refactor code to make it easier to add new recipes in the future. As it is, adding more build types will just require copying thousands of lines of code every time.
Perhaps more of a thought for the future: it would have been nice to consider creating something more generic or proposing some refactor that would have made it more straight forward to then add the rock builds (and all other builds). It definitely doesn't seem like an easy task, but looking at this code it seems like there are some opportunities for it.
Perhaps now that we did this exercise we have a better insight in how to improve it for next build type we add? Would be nice to for example write down some sort of guidelines or learnings for next time

Another topic to bring up is how large this MP is which makes it hard to review. For a next build type we add, maybe there is a way to separate concerns even more than this does. Perhaps part of the learnings/guidelines.
There are so many moving parts and I can't tell if something is missing or something is extra.

Last thing: I feel like it's frustrating that there are some oddities in the code, but looking through the charm code (and other build types) it just seems to be the way to make it coherent. I don't think we should address them in this MP, but... We should eventually address them. Some of my comments are about some of these.


Diff comments:

> diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
> index a1669be..28afbf3 100644
> --- a/lib/lp/code/model/gitrepository.py
> +++ b/lib/lp/code/model/gitrepository.py
> @@ -2004,6 +2005,15 @@ class GitRepository(
>                      self,
>                  )
>              )
> +        if not getUtility(IRockRecipeSet).findByGitRepository(self).is_empty():

I feel like this could easily be refactored into something like:

```
for every recipe type:
    if recipe exists for this repo:
        ...
```

instead of having each new recipe creating a new entry here...
Overall it would be great if we started making it easier to add new recipes in the future by doing some of these types of bits. Not just here but throughout the code base

> +            alteration_operations.append(
> +                DeletionCallable(
> +                    None,
> +                    msg("Some rock recipes build from this repository."),
> +                    getUtility(IRockRecipeSet).detachFromGitRepository,
> +                    self,
> +                )
> +            )
>  
>          return (alteration_operations, deletion_operations)
>  
> diff --git a/lib/lp/rocks/interfaces/rockrecipe.py b/lib/lp/rocks/interfaces/rockrecipe.py
> new file mode 100644
> index 0000000..d7797bd
> --- /dev/null
> +++ b/lib/lp/rocks/interfaces/rockrecipe.py
> @@ -0,0 +1,500 @@
> +# Copyright 2024 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Rock recipe interfaces."""
> +
> +__all__ = [
> +    "BadRockRecipeSource",
> +    "BadRockRecipeSearchContext",
> +    "ROCK_RECIPE_ALLOW_CREATE",
> +    "ROCK_RECIPE_PRIVATE_FEATURE_FLAG",
> +    "RockRecipeBuildRequestStatus",
> +    "RockRecipeFeatureDisabled",
> +    "RockRecipeNotOwner",
> +    "RockRecipePrivacyMismatch",
> +    "RockRecipePrivateFeatureDisabled",
> +    "DuplicateRockRecipeName",
> +    "IRockRecipe",
> +    "IRockRecipeBuildRequest",
> +    "IRockRecipeSet",
> +    "NoSourceForRockRecipe",
> +    "NoSuchRockRecipe",
> +]
> +
> +import http.client
> +
> +from lazr.enum import EnumeratedType, Item
> +from lazr.restful.declarations import error_status, exported
> +from lazr.restful.fields import Reference, ReferenceChoice
> +from zope.interface import Interface
> +from zope.schema import (
> +    Bool,
> +    Choice,
> +    Datetime,
> +    Dict,
> +    Int,
> +    List,
> +    Set,
> +    Text,
> +    TextLine,
> +)
> +from zope.security.interfaces import Unauthorized
> +
> +from lp import _
> +from lp.app.enums import InformationType
> +from lp.app.errors import NameLookupFailed
> +from lp.app.interfaces.informationtype import IInformationType
> +from lp.app.validators.name import name_validator
> +from lp.app.validators.path import path_does_not_escape
> +from lp.code.interfaces.gitref import IGitRef
> +from lp.code.interfaces.gitrepository import IGitRepository
> +from lp.registry.interfaces.product import IProduct
> +from lp.services.fields import PersonChoice, PublicPersonChoice
> +from lp.snappy.validators.channels import channels_validator
> +
> +ROCK_RECIPE_ALLOW_CREATE = "rock.recipe.create.enabled"
> +ROCK_RECIPE_PRIVATE_FEATURE_FLAG = "rock.recipe.allow_private"
> +
> +
> +@error_status(http.client.UNAUTHORIZED)
> +class RockRecipeFeatureDisabled(Unauthorized):
> +    """Only certain users can create new rock recipes."""
> +
> +    def __init__(self):
> +        super().__init__(
> +            "You do not have permission to create new rock recipes."
> +        )
> +
> +
> +@error_status(http.client.UNAUTHORIZED)
> +class RockRecipePrivateFeatureDisabled(Unauthorized):
> +    """Only certain users can create private rock recipes."""
> +
> +    def __init__(self):
> +        super().__init__(
> +            "You do not have permission to create private rock recipes."
> +        )
> +
> +
> +@error_status(http.client.BAD_REQUEST)
> +class DuplicateRockRecipeName(Exception):
> +    """Raised for rock recipes with duplicate project/owner/name."""
> +
> +    def __init__(self):
> +        super().__init__(
> +            "There is already a rock recipe with the same project, owner, "
> +            "and name."
> +        )
> +
> +
> +@error_status(http.client.UNAUTHORIZED)
> +class RockRecipeNotOwner(Unauthorized):
> +    """The registrant/requester is not the owner or a member of its team."""
> +
> +
> +class NoSuchRockRecipe(NameLookupFailed):
> +    """The requested rock recipe does not exist."""
> +
> +    _message_prefix = "No such rock recipe with this owner and project"
> +
> +
> +@error_status(http.client.BAD_REQUEST)
> +class NoSourceForRockRecipe(Exception):
> +    """Rock recipes must have a source (Git branch)."""
> +
> +    def __init__(self):
> +        super().__init__("New rock recipes must have a Git branch.")
> +
> +
> +@error_status(http.client.BAD_REQUEST)
> +class BadRockRecipeSource(Exception):
> +    """The elements of the source for a rock recipe are inconsistent."""
> +
> +
> +@error_status(http.client.BAD_REQUEST)
> +class RockRecipePrivacyMismatch(Exception):
> +    """Rock recipe privacy does not match its content."""
> +
> +    def __init__(self, message=None):
> +        super().__init__(
> +            message
> +            or "Rock recipe contains private information and cannot be public."
> +        )
> +
> +
> +class BadRockRecipeSearchContext(Exception):
> +    """The context is not valid for a rock recipe search."""
> +
> +
> +class RockRecipeBuildRequestStatus(EnumeratedType):

Not proposing to change that in this MP because I think it goes against the way this is being done, but I think these statuses and the exceptions also feel like something that could be generic for many build types

> +    """The status of a request to build a rock recipe."""
> +
> +    PENDING = Item(
> +        """
> +        Pending
> +
> +        This rock recipe build request is pending.
> +        """
> +    )
> +
> +    FAILED = Item(
> +        """
> +        Failed
> +
> +        This rock recipe build request failed.
> +        """
> +    )
> +
> +    COMPLETED = Item(
> +        """
> +        Completed
> +
> +        This rock recipe build request completed successfully.
> +        """
> +    )
> +
> +
> +class IRockRecipeBuildRequest(Interface):
> +    """A request to build a rock recipe."""
> +
> +    id = Int(title=_("ID"), required=True, readonly=True)
> +
> +    date_requested = Datetime(
> +        title=_("The time when this request was made"),
> +        required=True,
> +        readonly=True,
> +    )
> +
> +    date_finished = Datetime(
> +        title=_("The time when this request finished"),
> +        required=False,
> +        readonly=True,
> +    )
> +
> +    recipe = Reference(
> +        # Really IRockRecipe.
> +        Interface,
> +        title=_("Rock recipe"),
> +        required=True,
> +        readonly=True,
> +    )
> +
> +    status = Choice(
> +        title=_("Status"),
> +        vocabulary=RockRecipeBuildRequestStatus,
> +        required=True,
> +        readonly=True,
> +    )
> +
> +    error_message = TextLine(
> +        title=_("Error message"), required=True, readonly=True
> +    )
> +
> +    channels = Dict(
> +        title=_("Source snap channels for builds produced by this request"),

Should it say 'rock channels'? If I'm missing something and it's indeed snap channels then we could consider renaming it to `snap_channels` to be clearer.

>From my limited understanding of how rocks will work, the concept of channels might be used for their deployment.

If this is for `snap channels` - is this really a requirement/feature we want for the rock builds? (perhaps it is, I'm just challenging it for the sake of discussion)

> +        key_type=TextLine(),
> +        required=False,
> +        readonly=True,
> +    )
> +
> +    architectures = Set(
> +        title=_("If set, this request is limited to these architecture tags"),
> +        value_type=TextLine(),
> +        required=False,
> +        readonly=True,
> +    )
> +
> +
> +class IRockRecipeView(Interface):
> +    """`IRockRecipe` attributes that require launchpad.View permission."""
> +
> +    id = Int(title=_("ID"), required=True, readonly=True)
> +
> +    date_created = Datetime(
> +        title=_("Date created"), required=True, readonly=True
> +    )
> +    date_last_modified = Datetime(
> +        title=_("Date last modified"), required=True, readonly=True
> +    )
> +
> +    registrant = PublicPersonChoice(
> +        title=_("Registrant"),
> +        required=True,
> +        readonly=True,
> +        vocabulary="ValidPersonOrTeam",
> +        description=_("The person who registered this rock recipe."),
> +    )
> +
> +    private = Bool(
> +        title=_("Private"),
> +        required=False,
> +        readonly=False,
> +        description=_("Whether this rock recipe is private."),
> +    )
> +
> +    def getAllowedInformationTypes(user):
> +        """Get a list of acceptable `InformationType`s for this rock recipe.
> +
> +        If the user is a Launchpad admin, any type is acceptable.
> +        """
> +
> +    def visibleByUser(user):

Not something for this MP since this seems to be a pattern everywhere in our code base, but why do we skip the `self` parameter in the interface when the model method uses it? Is there a reason for it? I find this quite odd

> +        """Can the specified user see this rock recipe?"""
> +
> +    def requestBuilds(requester, channels=None, architectures=None):
> +        """Request that the rock recipe be built.
> +
> +        This is an asynchronous operation; once the operation has finished,
> +        the resulting build request's C{status} will be "Completed" and its
> +        C{builds} collection will return the resulting builds.
> +
> +        :param requester: The person requesting the builds.
> +        :param channels: A dictionary mapping snap names to channels to use

(Related to the comment above about `snap channels`)

> +            for these builds.
> +        :param architectures: If not None, limit builds to architectures
> +            with these architecture tags (in addition to any other
> +            applicable constraints).
> +        :return: An `IRockRecipeBuildRequest`.
> +        """
> +
> +    def getBuildRequest(job_id):
> +        """Get an asynchronous build request by ID.
> +
> +        :param job_id: The ID of the build request.
> +        :return: `IRockRecipeBuildRequest`.
> +        """
> +
> +
> +class IRockRecipeEdit(Interface):
> +    """`IRockRecipe` methods that require launchpad.Edit permission."""
> +
> +    def destroySelf():
> +        """Delete this rock recipe, provided that it has no builds."""
> +
> +
> +class IRockRecipeEditableAttributes(Interface):
> +    """`IRockRecipe` attributes that can be edited.
> +
> +    These attributes need launchpad.View to see, and launchpad.Edit to change.
> +    """
> +
> +    owner = exported(
> +        PersonChoice(
> +            title=_("Owner"),
> +            required=True,
> +            readonly=False,
> +            vocabulary="AllUserTeamsParticipationPlusSelf",
> +            description=_("The owner of this rock recipe."),
> +        )
> +    )
> +
> +    project = ReferenceChoice(
> +        title=_("The project that this rock recipe is associated with"),
> +        schema=IProduct,
> +        vocabulary="Product",
> +        required=True,
> +        readonly=False,
> +    )
> +
> +    name = TextLine(
> +        title=_("Rock recipe name"),
> +        required=True,
> +        readonly=False,
> +        constraint=name_validator,
> +        description=_("The name of the rock recipe."),
> +    )
> +
> +    description = Text(
> +        title=_("Description"),
> +        required=False,
> +        readonly=False,
> +        description=_("A description of the rock recipe."),
> +    )
> +
> +    git_repository = ReferenceChoice(
> +        title=_("Git repository"),
> +        schema=IGitRepository,
> +        vocabulary="GitRepository",
> +        required=False,
> +        readonly=True,
> +        description=_(
> +            "A Git repository with a branch containing a rockcraft.yaml "
> +            "recipe."
> +        ),
> +    )
> +
> +    git_path = TextLine(
> +        title=_("Git branch path"),
> +        required=False,
> +        readonly=False,
> +        description=_(
> +            "The path of the Git branch containing a rockcraft.yaml recipe."
> +        ),
> +    )
> +
> +    git_ref = Reference(
> +        IGitRef,
> +        title=_("Git branch"),
> +        required=False,
> +        readonly=False,
> +        description=_("The Git branch containing a rockcraft.yaml recipe."),
> +    )
> +
> +    build_path = TextLine(
> +        title=_("Build path"),
> +        description=_(
> +            "Subdirectory within the branch containing rockcraft.yaml."
> +        ),
> +        constraint=path_does_not_escape,
> +        required=False,
> +        readonly=False,
> +    )
> +    information_type = Choice(
> +        title=_("Information type"),
> +        vocabulary=InformationType,
> +        required=True,
> +        readonly=False,
> +        default=InformationType.PUBLIC,
> +        description=_(
> +            "The type of information contained in this rock recipe."
> +        ),
> +    )
> +
> +    auto_build = Bool(
> +        title=_("Automatically build when branch changes"),
> +        required=True,
> +        readonly=False,
> +        description=_(
> +            "Whether this rock recipe is built automatically when the branch "
> +            "containing its rockcraft.yaml recipe changes."
> +        ),
> +    )
> +
> +    auto_build_channels = Dict(
> +        title=_("Source snap channels for automatic builds"),
> +        key_type=TextLine(),
> +        required=False,
> +        readonly=False,
> +        description=_(
> +            "A dictionary mapping snap names to channels to use when building "

Related to comment above, just want to make sure we mean snaps and not rocks here (since channels seems a little confusing to me if both will use them)

> +            "this rock recipe.  Currently only 'core', 'core18', 'core20', "
> +            "and 'rockcraft' keys are supported."
> +        ),
> +    )
> +
> +    is_stale = Bool(
> +        title=_("Rock recipe is stale and is due to be rebuilt."),
> +        required=True,
> +        readonly=True,
> +    )
> +
> +    store_upload = Bool(
> +        title=_("Automatically upload to store"),
> +        required=True,
> +        readonly=False,
> +        description=_(
> +            "Whether builds of this rock recipe are automatically uploaded "
> +            "to the store."
> +        ),
> +    )
> +
> +    store_name = TextLine(
> +        title=_("Registered store name"),
> +        required=False,
> +        readonly=False,
> +        description=_("The registered name of this rock in the store."),
> +    )
> +
> +    store_secrets = List(
> +        value_type=TextLine(),
> +        title=_("Store upload tokens"),
> +        required=False,
> +        readonly=False,
> +        description=_(
> +            "Serialized secrets issued by the store and the login service to "
> +            "authorize uploads of this rock recipe."
> +        ),
> +    )
> +
> +    store_channels = List(
> +        title=_("Store channels"),
> +        required=False,
> +        readonly=False,
> +        constraint=channels_validator,

This method currently comes from the `snappy` which doesn't feel right. Perhaps it belong to some utilities type of dir?

Or perhaps it should be the start of a shared 'builds' dir that could contain a lot of the code in common between build types... Though having that dir would only make sense if we find there is more common code between builds (I think there is a lot of common code, but we would need some refactoring which doesn't feel like it should be part of this MP).

Thoughts?

> +        description=_(
> +            "Channels to release this rock to after uploading it to the "
> +            "store. A channel is defined by a combination of an optional "
> +            "track, a risk, and an optional branch, e.g. "
> +            "'2.1/stable/fix-123', '2.1/stable', 'stable/fix-123', or "
> +            "'stable'."
> +        ),
> +    )
> +
> +
> +class IRockRecipeAdminAttributes(Interface):
> +    """`IRockRecipe` attributes that can be edited by admins.
> +
> +    These attributes need launchpad.View to see, and launchpad.Admin to change.
> +    """
> +
> +    require_virtualized = Bool(
> +        title=_("Require virtualized builders"),
> +        required=True,
> +        readonly=False,
> +        description=_("Only build this rock recipe on virtual builders."),
> +    )
> +
> +
> +class IRockRecipe(
> +    IRockRecipeView,
> +    IRockRecipeEdit,
> +    IRockRecipeEditableAttributes,
> +    IRockRecipeAdminAttributes,
> +    IInformationType,
> +):
> +    """A buildable rock recipe."""
> +
> +
> +class IRockRecipeSet(Interface):
> +    """A utility to create and access rock recipes."""
> +
> +    def new(
> +        registrant,
> +        owner,
> +        project,
> +        name,
> +        description=None,
> +        git_ref=None,
> +        build_path=None,
> +        require_virtualized=True,
> +        information_type=InformationType.PUBLIC,
> +        auto_build=False,
> +        auto_build_channels=None,
> +        store_upload=False,
> +        store_name=None,
> +        store_secrets=None,
> +        store_channels=None,
> +        date_created=None,
> +    ):
> +        """Create an `IRockRecipe`."""
> +
> +    def getByName(owner, project, name):
> +        """Returns the appropriate `IRockRecipe` for the given objects."""
> +
> +    def isValidInformationType(information_type, owner, git_ref=None):
> +        """Whether the information type context is valid."""
> +
> +    def findByGitRepository(repository, paths=None):
> +        """Return all rock recipes for the given Git repository.
> +
> +        :param repository: An `IGitRepository`.
> +        :param paths: If not None, only return rock recipes for one of
> +            these Git reference paths.
> +        """
> +
> +    def detachFromGitRepository(repository):
> +        """Detach all rock recipes from the given Git repository.
> +
> +        After this, any rock recipes that previously used this repository
> +        will have no source and so cannot dispatch new builds.
> +        """
> diff --git a/lib/lp/rocks/interfaces/rockrecipejob.py b/lib/lp/rocks/interfaces/rockrecipejob.py
> new file mode 100644
> index 0000000..742b7ed
> --- /dev/null
> +++ b/lib/lp/rocks/interfaces/rockrecipejob.py
> @@ -0,0 +1,127 @@
> +# Copyright 2024 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Rock recipe job interfaces."""
> +
> +__all__ = [
> +    "IRockRecipeJob",
> +    "IRockRecipeRequestBuildsJob",
> +    "IRockRecipeRequestBuildsJobSource",
> +]
> +
> +from lazr.restful.fields import Reference
> +from zope.interface import Attribute, Interface
> +from zope.schema import Datetime, Dict, Set, TextLine
> +
> +from lp import _
> +from lp.registry.interfaces.person import IPerson
> +from lp.rocks.interfaces.rockrecipe import IRockRecipe, IRockRecipeBuildRequest
> +from lp.services.job.interfaces.job import IJob, IJobSource, IRunnableJob
> +
> +
> +class IRockRecipeJob(Interface):
> +    """A job related to a rock recipe."""
> +
> +    job = Reference(
> +        title=_("The common Job attributes."),
> +        schema=IJob,
> +        required=True,
> +        readonly=True,
> +    )
> +
> +    recipe = Reference(
> +        title=_("The rock recipe to use for this job."),
> +        schema=IRockRecipe,
> +        required=True,
> +        readonly=True,
> +    )
> +
> +    metadata = Attribute(_("A dict of data about the job."))
> +
> +
> +class IRockRecipeRequestBuildsJob(IRunnableJob):
> +    """A Job that processes a request for builds of a rock recipe."""
> +
> +    requester = Reference(
> +        title=_("The person requesting the builds."),
> +        schema=IPerson,
> +        required=True,
> +        readonly=True,
> +    )
> +
> +    channels = Dict(
> +        title=_("Source snap channels to use for these builds."),

(similar as comments above, sorry to be leaving so many of these when maybe are just the way they are supposed to be,just so it's not missed since I'm here :)

> +        description=_(
> +            "A dictionary mapping snap names to channels to use for these "
> +            "builds.  Currently only 'core', 'core18', 'core20', and "
> +            "'rockcraft' keys are supported."
> +        ),
> +        key_type=TextLine(),
> +        required=False,
> +        readonly=True,
> +    )
> +
> +    architectures = Set(
> +        title=_("If set, limit builds to these architecture tags."),
> +        value_type=TextLine(),
> +        required=False,
> +        readonly=True,
> +    )
> +
> +    date_created = Datetime(
> +        title=_("Time when this job was created."),
> +        required=True,
> +        readonly=True,
> +    )
> +
> +    date_finished = Datetime(
> +        title=_("Time when this job finished."), required=True, readonly=True
> +    )
> +
> +    error_message = TextLine(
> +        title=_("Error message resulting from running this job."),
> +        required=False,
> +        readonly=True,
> +    )
> +
> +    build_request = Reference(
> +        title=_("The build request corresponding to this job."),
> +        schema=IRockRecipeBuildRequest,
> +        required=True,
> +        readonly=True,
> +    )
> +
> +
> +class IRockRecipeRequestBuildsJobSource(IJobSource):
> +
> +    def create(recipe, requester, channels=None, architectures=None):
> +        """Request builds of a rock recipe.
> +
> +        :param recipe: The rock recipe to build.
> +        :param requester: The person requesting the builds.
> +        :param channels: A dictionary mapping snap names to channels to use
> +            for these builds.
> +        :param architectures: If not None, limit builds to architectures
> +            with these architecture tags (in addition to any other
> +            applicable constraints).
> +        """
> +
> +    def findByRecipe(recipe, statuses=None, job_ids=None):
> +        """Find jobs for a rock recipe.
> +
> +        :param recipe: A rock recipe to search for.
> +        :param statuses: An optional iterable of `JobStatus`es to search for.
> +        :param job_ids: An optional iterable of job IDs to search for.
> +        :return: A sequence of `RockRecipeRequestBuildsJob`s with the
> +            specified recipe.
> +        """
> +
> +    def getByRecipeAndID(recipe, job_id):
> +        """Get a job by rock recipe and job ID.
> +
> +        :return: The `RockRecipeRequestBuildsJob` with the specified recipe
> +            and ID.
> +        :raises: `NotFoundError` if there is no job with the specified
> +            recipe and ID, or its `job_type` is not
> +            `RockRecipeJobType.REQUEST_BUILDS`.
> +        """
> diff --git a/lib/lp/rocks/model/rockrecipe.py b/lib/lp/rocks/model/rockrecipe.py
> new file mode 100644
> index 0000000..9e6521c
> --- /dev/null
> +++ b/lib/lp/rocks/model/rockrecipe.py
> @@ -0,0 +1,443 @@
> +# Copyright 2024 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Rock recipes."""
> +
> +__all__ = [
> +    "RockRecipe",
> +]
> +
> +from datetime import timezone
> +
> +from storm.databases.postgres import JSON
> +from storm.locals import Bool, DateTime, Int, Reference, Unicode
> +from zope.component import getUtility
> +from zope.interface import implementer
> +from zope.security.proxy import removeSecurityProxy
> +
> +from lp.app.enums import (
> +    FREE_INFORMATION_TYPES,
> +    PUBLIC_INFORMATION_TYPES,
> +    InformationType,
> +)
> +from lp.code.model.gitrepository import GitRepository
> +from lp.registry.errors import PrivatePersonLinkageError
> +from lp.registry.interfaces.person import validate_public_person
> +from lp.rocks.interfaces.rockrecipe import (
> +    ROCK_RECIPE_ALLOW_CREATE,
> +    ROCK_RECIPE_PRIVATE_FEATURE_FLAG,
> +    DuplicateRockRecipeName,
> +    IRockRecipe,
> +    IRockRecipeBuildRequest,
> +    IRockRecipeSet,
> +    NoSourceForRockRecipe,
> +    RockRecipeBuildRequestStatus,
> +    RockRecipeFeatureDisabled,
> +    RockRecipeNotOwner,
> +    RockRecipePrivacyMismatch,
> +    RockRecipePrivateFeatureDisabled,
> +)
> +from lp.rocks.interfaces.rockrecipejob import IRockRecipeRequestBuildsJobSource
> +from lp.services.database.constants import DEFAULT, UTC_NOW
> +from lp.services.database.enumcol import DBEnum
> +from lp.services.database.interfaces import IPrimaryStore, IStore
> +from lp.services.database.stormbase import StormBase
> +from lp.services.features import getFeatureFlag
> +from lp.services.job.interfaces.job import JobStatus
> +from lp.services.propertycache import cachedproperty, get_property_cache
> +
> +
> +def rock_recipe_modified(recipe, event):
> +    """Update the date_last_modified property when a rock recipe is modified.
> +
> +    This method is registered as a subscriber to `IObjectModifiedEvent`
> +    events on rock recipes.
> +    """
> +    removeSecurityProxy(recipe).date_last_modified = UTC_NOW
> +
> +
> +@implementer(IRockRecipeBuildRequest)
> +class RockRecipeBuildRequest:
> +    """See `IRockRecipeBuildRequest`.
> +
> +    This is not directly backed by a database table; instead, it is a
> +    webservice-friendly view of an asynchronous build request.
> +    """
> +
> +    def __init__(self, recipe, id):
> +        self.recipe = recipe
> +        self.id = id
> +
> +    @classmethod
> +    def fromJob(cls, job):
> +        """See `IRockRecipeBuildRequest`."""
> +        request = cls(job.recipe, job.job_id)
> +        get_property_cache(request)._job = job
> +        return request
> +
> +    @cachedproperty
> +    def _job(self):
> +        job_source = getUtility(IRockRecipeRequestBuildsJobSource)
> +        return job_source.getByRecipeAndID(self.recipe, self.id)
> +
> +    @property
> +    def date_requested(self):
> +        """See `IRockRecipeBuildRequest`."""
> +        return self._job.date_created
> +
> +    @property
> +    def date_finished(self):
> +        """See `IRockRecipeBuildRequest`."""
> +        return self._job.date_finished
> +
> +    @property
> +    def status(self):
> +        """See `IRockRecipeBuildRequest`."""
> +        status_map = {
> +            JobStatus.WAITING: RockRecipeBuildRequestStatus.PENDING,
> +            JobStatus.RUNNING: RockRecipeBuildRequestStatus.PENDING,
> +            JobStatus.COMPLETED: RockRecipeBuildRequestStatus.COMPLETED,
> +            JobStatus.FAILED: RockRecipeBuildRequestStatus.FAILED,
> +            JobStatus.SUSPENDED: RockRecipeBuildRequestStatus.PENDING,
> +        }
> +        return status_map[self._job.job.status]
> +
> +    @property
> +    def error_message(self):
> +        """See `IRockRecipeBuildRequest`."""
> +        return self._job.error_message
> +
> +    @property
> +    def channels(self):
> +        """See `IRockRecipeBuildRequest`."""
> +        return self._job.channels
> +
> +    @property
> +    def architectures(self):
> +        """See `IRockRecipeBuildRequest`."""
> +        return self._job.architectures
> +
> +
> +@implementer(IRockRecipe)
> +class RockRecipe(StormBase):
> +    """See `IRockRecipe`."""
> +
> +    __storm_table__ = "RockRecipe"
> +
> +    id = Int(primary=True)
> +
> +    date_created = DateTime(
> +        name="date_created", tzinfo=timezone.utc, allow_none=False
> +    )
> +    date_last_modified = DateTime(
> +        name="date_last_modified", tzinfo=timezone.utc, allow_none=False
> +    )
> +
> +    registrant_id = Int(name="registrant", allow_none=False)
> +    registrant = Reference(registrant_id, "Person.id")
> +
> +    def _validate_owner(self, attr, value):
> +        if not self.private:
> +            try:
> +                validate_public_person(self, attr, value)

This feels like one of the many places where I get confused reading code, only to then realize it's just how it is for all other recipes... What is `attr` and `value` here? I had to go through the method to actually understand what it was.

> +            except PrivatePersonLinkageError:
> +                raise RockRecipePrivacyMismatch(
> +                    "A public rock recipe cannot have a private owner."
> +                )
> +        return value
> +
> +    owner_id = Int(name="owner", allow_none=False, validator=_validate_owner)
> +    owner = Reference(owner_id, "Person.id")
> +
> +    project_id = Int(name="project", allow_none=False)
> +    project = Reference(project_id, "Product.id")
> +
> +    name = Unicode(name="name", allow_none=False)
> +
> +    description = Unicode(name="description", allow_none=True)
> +
> +    def _validate_git_repository(self, attr, value):
> +        if not self.private and value is not None:
> +            if IStore(GitRepository).get(GitRepository, value).private:
> +                raise RockRecipePrivacyMismatch(
> +                    "A public rock recipe cannot have a private repository."
> +                )
> +        return value
> +
> +    git_repository_id = Int(
> +        name="git_repository",
> +        allow_none=True,
> +        validator=_validate_git_repository,
> +    )
> +    git_repository = Reference(git_repository_id, "GitRepository.id")
> +
> +    git_path = Unicode(name="git_path", allow_none=True)
> +
> +    build_path = Unicode(name="build_path", allow_none=True)
> +
> +    require_virtualized = Bool(name="require_virtualized")
> +
> +    def _valid_information_type(self, attr, value):
> +        if not getUtility(IRockRecipeSet).isValidInformationType(
> +            value, self.owner, self.git_ref
> +        ):
> +            raise RockRecipePrivacyMismatch
> +        return value
> +
> +    information_type = DBEnum(
> +        enum=InformationType,
> +        default=InformationType.PUBLIC,
> +        name="information_type",
> +        validator=_valid_information_type,
> +        allow_none=False,
> +    )
> +
> +    auto_build = Bool(name="auto_build", allow_none=False)
> +
> +    auto_build_channels = JSON("auto_build_channels", allow_none=True)
> +
> +    is_stale = Bool(name="is_stale", allow_none=False)
> +
> +    store_upload = Bool(name="store_upload", allow_none=False)

Are we going to implement the bits to upload rocks to stores as part of all of these changes?

Good if so, but overall it feels as though we could have cut this MP into more topical bits (which seems very hard to do, but perhaps would be simpler to move things along). E.g. we would add 'store upload' feature after adding everything else...

Having it here suggests that we will have it right from the start and have to implement the logic to upload it to the (not yet existant?) rock store

> +
> +    store_name = Unicode(name="store_name", allow_none=True)
> +
> +    store_secrets = JSON("store_secrets", allow_none=True)
> +
> +    _store_channels = JSON("store_channels", allow_none=True)
> +
> +    def __init__(
> +        self,
> +        registrant,
> +        owner,
> +        project,
> +        name,
> +        description=None,
> +        git_ref=None,
> +        build_path=None,
> +        require_virtualized=True,
> +        information_type=InformationType.PUBLIC,
> +        auto_build=False,
> +        auto_build_channels=None,
> +        store_upload=False,
> +        store_name=None,
> +        store_secrets=None,
> +        store_channels=None,
> +        date_created=DEFAULT,
> +    ):
> +        """Construct a `RockRecipe`."""
> +        if not getFeatureFlag(ROCK_RECIPE_ALLOW_CREATE):
> +            raise RockRecipeFeatureDisabled()
> +        super().__init__()
> +
> +        # Set this first for use by other validators.
> +        self.information_type = information_type
> +
> +        self.date_created = date_created
> +        self.date_last_modified = date_created
> +        self.registrant = registrant
> +        self.owner = owner
> +        self.project = project
> +        self.name = name
> +        self.description = description
> +        self.git_ref = git_ref
> +        self.build_path = build_path
> +        self.require_virtualized = require_virtualized
> +        self.auto_build = auto_build
> +        self.auto_build_channels = auto_build_channels
> +        self.store_upload = store_upload
> +        self.store_name = store_name
> +        self.store_secrets = store_secrets
> +        self.store_channels = store_channels
> +
> +    def __repr__(self):
> +        return "<RockRecipe ~%s/%s/+rock/%s>" % (
> +            self.owner.name,
> +            self.project.name,
> +            self.name,
> +        )
> +
> +    @property
> +    def private(self):
> +        """See `IRockRecipe`."""
> +        return self.information_type not in PUBLIC_INFORMATION_TYPES
> +
> +    @property
> +    def git_ref(self):
> +        """See `IRockRecipe`."""
> +        if self.git_repository is not None:
> +            return self.git_repository.getRefByPath(self.git_path)
> +        else:
> +            return None
> +
> +    @git_ref.setter
> +    def git_ref(self, value):
> +        """See `IRockRecipe`."""
> +        if value is not None:
> +            self.git_repository = value.repository
> +            self.git_path = value.path
> +        else:
> +            self.git_repository = None
> +            self.git_path = None
> +
> +    @property
> +    def store_channels(self):
> +        """See `IRockRecipe`."""
> +        return self._store_channels or []
> +
> +    @store_channels.setter
> +    def store_channels(self, value):
> +        """See `IRockRecipe`."""
> +        self._store_channels = value or None
> +
> +    def getAllowedInformationTypes(self, user):
> +        """See `IRockRecipe`."""
> +        # XXX jugmac00 2024-08-29: Only allow free information types until
> +        # we have more privacy infrastructure in place.
> +        return FREE_INFORMATION_TYPES
> +
> +    def visibleByUser(self, user):
> +        """See `IRockRecipe`."""
> +        if self.information_type in PUBLIC_INFORMATION_TYPES:
> +            return True
> +        # XXX jugmac00 2024-08-29: Finish implementing this once we have
> +        # more privacy infrastructure.
> +        return False
> +
> +    def _checkRequestBuild(self, requester):
> +        """May `requester` request builds of this rock recipe?"""
> +        if not requester.inTeam(self.owner):
> +            raise RockRecipeNotOwner(
> +                "%s cannot create rock recipe builds owned by %s."
> +                % (requester.display_name, self.owner.display_name)
> +            )
> +
> +    def requestBuilds(self, requester, channels=None, architectures=None):
> +        """See `IRockRecipe`."""
> +        self._checkRequestBuild(requester)
> +        job = getUtility(IRockRecipeRequestBuildsJobSource).create(
> +            self, requester, channels=channels, architectures=architectures
> +        )
> +        return self.getBuildRequest(job.job_id)
> +
> +    def getBuildRequest(self, job_id):
> +        """See `IRockRecipe`."""
> +        return RockRecipeBuildRequest(self, job_id)
> +
> +    def destroySelf(self):
> +        """See `IRockRecipe`."""
> +        IStore(RockRecipe).remove(self)
> +
> +
> +@implementer(IRockRecipeSet)
> +class RockRecipeSet:
> +    """See `IRockRecipeSet`."""
> +
> +    def new(
> +        self,
> +        registrant,
> +        owner,
> +        project,
> +        name,
> +        description=None,
> +        git_ref=None,
> +        build_path=None,
> +        require_virtualized=True,
> +        information_type=InformationType.PUBLIC,
> +        auto_build=False,
> +        auto_build_channels=None,
> +        store_upload=False,
> +        store_name=None,
> +        store_secrets=None,
> +        store_channels=None,
> +        date_created=DEFAULT,
> +    ):
> +        """See `IRockRecipeSet`."""
> +        if not registrant.inTeam(owner):
> +            if owner.is_team:
> +                raise RockRecipeNotOwner(
> +                    "%s is not a member of %s."
> +                    % (registrant.displayname, owner.displayname)
> +                )
> +            else:
> +                raise RockRecipeNotOwner(
> +                    "%s cannot create rock recipes owned by %s."
> +                    % (registrant.displayname, owner.displayname)
> +                )
> +
> +        if git_ref is None:
> +            raise NoSourceForRockRecipe
> +        if self.getByName(owner, project, name) is not None:
> +            raise DuplicateRockRecipeName
> +
> +        # The relevant validators will do their own checks as well, but we
> +        # do a single up-front check here in order to avoid an
> +        # IntegrityError due to exceptions being raised during object
> +        # creation and to ensure that everything relevant is in the Storm
> +        # cache.
> +        if not self.isValidInformationType(information_type, owner, git_ref):
> +            raise RockRecipePrivacyMismatch
> +        store = IPrimaryStore(RockRecipe)
> +        recipe = RockRecipe(
> +            registrant,
> +            owner,
> +            project,
> +            name,
> +            description=description,
> +            git_ref=git_ref,
> +            build_path=build_path,
> +            require_virtualized=require_virtualized,
> +            information_type=information_type,
> +            auto_build=auto_build,
> +            auto_build_channels=auto_build_channels,
> +            store_upload=store_upload,
> +            store_name=store_name,
> +            store_secrets=store_secrets,
> +            store_channels=store_channels,
> +            date_created=date_created,
> +        )
> +        store.add(recipe)
> +
> +        return recipe
> +
> +    def getByName(self, owner, project, name):
> +        """See `IRockRecipeSet`."""
> +        return (
> +            IStore(RockRecipe)
> +            .find(RockRecipe, owner=owner, project=project, name=name)
> +            .one()
> +        )
> +
> +    def isValidInformationType(self, information_type, owner, git_ref=None):
> +        """See `IRockRecipeSet`."""
> +        private = information_type not in PUBLIC_INFORMATION_TYPES
> +        if private:
> +            # If appropriately enabled via feature flag.
> +            if not getFeatureFlag(ROCK_RECIPE_PRIVATE_FEATURE_FLAG):
> +                raise RockRecipePrivateFeatureDisabled
> +            return True
> +
> +        # Public rock recipes with private sources are not allowed.
> +        if git_ref is not None and git_ref.private:
> +            return False
> +
> +        # Public rock recipes owned by private teams are not allowed.
> +        if owner is not None and owner.private:
> +            return False
> +
> +        return True
> +
> +    def findByGitRepository(self, repository, paths=None):
> +        """See `IRockRecipeSet`."""
> +        clauses = [RockRecipe.git_repository == repository]
> +        if paths is not None:
> +            clauses.append(RockRecipe.git_path.is_in(paths))
> +        # XXX jugmac00 2024-08-29: Check permissions once we have some
> +        # privacy infrastructure.
> +        return IStore(RockRecipe).find(RockRecipe, *clauses)
> +
> +    def detachFromGitRepository(self, repository):
> +        """See `ICharmRecipeSet`."""

See _IRockRecipeSet_?

> +        self.findByGitRepository(repository).set(
> +            git_repository_id=None, git_path=None, date_last_modified=UTC_NOW
> +        )
> diff --git a/lib/lp/rocks/model/rockrecipejob.py b/lib/lp/rocks/model/rockrecipejob.py
> new file mode 100644
> index 0000000..3c8c0d0
> --- /dev/null
> +++ b/lib/lp/rocks/model/rockrecipejob.py
> @@ -0,0 +1,298 @@
> +# Copyright 2024 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Rock recipe jobs."""
> +
> +__all__ = [
> +    "RockRecipeJob",
> +    "RockRecipeJobType",
> +    "RockRecipeRequestBuildsJob",
> +]
> +
> +import transaction
> +from lazr.delegates import delegate_to
> +from lazr.enum import DBEnumeratedType, DBItem
> +from storm.databases.postgres import JSON
> +from storm.locals import Desc, Int, Reference
> +from zope.component import getUtility
> +from zope.interface import implementer, provider
> +
> +from lp.app.errors import NotFoundError
> +from lp.registry.interfaces.person import IPersonSet
> +from lp.rocks.interfaces.rockrecipejob import (
> +    IRockRecipeJob,
> +    IRockRecipeRequestBuildsJob,
> +    IRockRecipeRequestBuildsJobSource,
> +)
> +from lp.services.config import config
> +from lp.services.database.bulk import load_related
> +from lp.services.database.decoratedresultset import DecoratedResultSet
> +from lp.services.database.enumcol import DBEnum
> +from lp.services.database.interfaces import IPrimaryStore, IStore
> +from lp.services.database.stormbase import StormBase
> +from lp.services.job.model.job import EnumeratedSubclass, Job
> +from lp.services.job.runner import BaseRunnableJob
> +from lp.services.mail.sendmail import format_address_for_person
> +from lp.services.propertycache import cachedproperty
> +from lp.services.scripts import log
> +
> +
> +class RockRecipeJobType(DBEnumeratedType):
> +    """Values that `IRockRecipeJob.job_type` can take."""
> +
> +    REQUEST_BUILDS = DBItem(
> +        0,
> +        """
> +        Request builds
> +
> +        This job requests builds of a rock recipe.
> +        """,
> +    )
> +
> +
> +@implementer(IRockRecipeJob)
> +class RockRecipeJob(StormBase):
> +    """See `IRockRecipeJob`."""
> +
> +    __storm_table__ = "RockRecipeJob"
> +
> +    job_id = Int(name="job", primary=True, allow_none=False)
> +    job = Reference(job_id, "Job.id")
> +
> +    recipe_id = Int(name="recipe", allow_none=False)
> +    recipe = Reference(recipe_id, "RockRecipe.id")
> +
> +    job_type = DBEnum(
> +        name="job_type", enum=RockRecipeJobType, allow_none=False
> +    )
> +
> +    metadata = JSON("json_data", allow_none=False)
> +
> +    def __init__(self, recipe, job_type, metadata, **job_args):
> +        """Constructor.
> +
> +        Extra keyword arguments are used to construct the underlying Job
> +        object.
> +
> +        :param recipe: The `IRockRecipe` this job relates to.
> +        :param job_type: The `RockRecipeJobType` of this job.
> +        :param metadata: The type-specific variables, as a JSON-compatible
> +            dict.
> +        """
> +        super().__init__()
> +        self.job = Job(**job_args)
> +        self.recipe = recipe
> +        self.job_type = job_type
> +        self.metadata = metadata
> +
> +    def makeDerived(self):
> +        return RockRecipeJobDerived.makeSubclass(self)
> +
> +
> +@delegate_to(IRockRecipeJob)
> +class RockRecipeJobDerived(BaseRunnableJob, metaclass=EnumeratedSubclass):
> +
> +    def __init__(self, recipe_job):
> +        self.context = recipe_job
> +
> +    def __repr__(self):
> +        """An informative representation of the job."""
> +        return "<%s for ~%s/%s/+rock/%s>" % (
> +            self.__class__.__name__,
> +            self.recipe.owner.name,
> +            self.recipe.project.name,
> +            self.recipe.name,
> +        )
> +
> +    @classmethod
> +    def get(cls, job_id):
> +        """Get a job by id.
> +
> +        :return: The `RockRecipeJob` with the specified id, as the current
> +            `RockRecipeJobDerived` subclass.
> +        :raises: `NotFoundError` if there is no job with the specified id,
> +            or its `job_type` does not match the desired subclass.
> +        """
> +        recipe_job = IStore(RockRecipeJob).get(RockRecipeJob, job_id)
> +        if recipe_job.job_type != cls.class_job_type:
> +            raise NotFoundError(
> +                "No object found with id %d and type %s"
> +                % (job_id, cls.class_job_type.title)
> +            )
> +        return cls(recipe_job)
> +
> +    @classmethod
> +    def iterReady(cls):
> +        """See `IJobSource`."""
> +        jobs = IPrimaryStore(RockRecipeJob).find(
> +            RockRecipeJob,
> +            RockRecipeJob.job_type == cls.class_job_type,
> +            RockRecipeJob.job == Job.id,
> +            Job.id.is_in(Job.ready_jobs),
> +        )
> +        return (cls(job) for job in jobs)
> +
> +    def getOopsVars(self):

Related to comments above, this also feels like something that could be common for multiple build types

> +        """See `IRunnableJob`."""
> +        oops_vars = super().getOopsVars()
> +        oops_vars.extend(
> +            [
> +                ("job_id", self.context.job.id),
> +                ("job_type", self.context.job_type.title),
> +                ("recipe_owner_name", self.context.recipe.owner.name),
> +                ("recipe_project_name", self.context.recipe.project.name),
> +                ("recipe_name", self.context.recipe.name),
> +            ]
> +        )
> +        return oops_vars
> +
> +
> +@implementer(IRockRecipeRequestBuildsJob)
> +@provider(IRockRecipeRequestBuildsJobSource)
> +class RockRecipeRequestBuildsJob(RockRecipeJobDerived):
> +    """A Job that processes a request for builds of a rock recipe."""
> +
> +    class_job_type = RockRecipeJobType.REQUEST_BUILDS
> +
> +    max_retries = 5
> +
> +    config = config.IRockRecipeRequestBuildsJobSource
> +
> +    @classmethod
> +    def create(cls, recipe, requester, channels=None, architectures=None):
> +        """See `IRockRecipeRequestBuildsJobSource`."""
> +        metadata = {
> +            "requester": requester.id,
> +            "channels": channels,
> +            # Really a set or None, but sets aren't directly
> +            # JSON-serialisable.
> +            "architectures": (
> +                list(architectures) if architectures is not None else None
> +            ),
> +        }
> +        recipe_job = RockRecipeJob(recipe, cls.class_job_type, metadata)
> +        job = cls(recipe_job)
> +        job.celeryRunOnCommit()
> +        IStore(RockRecipeJob).flush()
> +        return job
> +
> +    @classmethod
> +    def findByRecipe(cls, recipe, statuses=None, job_ids=None):
> +        """See `IRockRecipeRequestBuildsJobSource`."""
> +        clauses = [
> +            RockRecipeJob.recipe == recipe,
> +            RockRecipeJob.job_type == cls.class_job_type,
> +        ]
> +        if statuses is not None:
> +            clauses.extend(
> +                [
> +                    RockRecipeJob.job == Job.id,
> +                    Job._status.is_in(statuses),
> +                ]
> +            )
> +        if job_ids is not None:
> +            clauses.append(RockRecipeJob.job_id.is_in(job_ids))
> +        recipe_jobs = (
> +            IStore(RockRecipeJob)
> +            .find(RockRecipeJob, *clauses)
> +            .order_by(Desc(RockRecipeJob.job_id))
> +        )
> +
> +        def preload_jobs(rows):
> +            load_related(Job, rows, ["job_id"])
> +
> +        return DecoratedResultSet(
> +            recipe_jobs,
> +            lambda recipe_job: cls(recipe_job),
> +            pre_iter_hook=preload_jobs,
> +        )
> +
> +    @classmethod
> +    def getByRecipeAndID(cls, recipe, job_id):
> +        """See `IRockRecipeRequestBuildsJobSource`."""
> +        recipe_job = (
> +            IStore(RockRecipeJob)
> +            .find(
> +                RockRecipeJob,
> +                RockRecipeJob.job_id == job_id,
> +                RockRecipeJob.recipe == recipe,
> +                RockRecipeJob.job_type == cls.class_job_type,
> +            )
> +            .one()
> +        )
> +        if recipe_job is None:
> +            raise NotFoundError(
> +                "No REQUEST_BUILDS job with ID %d found for %r"
> +                % (job_id, recipe)
> +            )
> +        return cls(recipe_job)
> +
> +    def getOperationDescription(self):
> +        return "requesting builds of %s" % self.recipe.name
> +
> +    def getErrorRecipients(self):
> +        if self.requester is None or self.requester.preferredemail is None:
> +            return []
> +        return [format_address_for_person(self.requester)]
> +
> +    @cachedproperty
> +    def requester(self):
> +        """See `IRockRecipeRequestBuildsJob`."""
> +        requester_id = self.metadata["requester"]
> +        return getUtility(IPersonSet).get(requester_id)
> +
> +    @property
> +    def channels(self):
> +        """See `IRockRecipeRequestBuildsJob`."""
> +        return self.metadata["channels"]
> +
> +    @property
> +    def architectures(self):
> +        """See `IRockRecipeRequestBuildsJob`."""
> +        architectures = self.metadata["architectures"]
> +        return set(architectures) if architectures is not None else None
> +
> +    @property
> +    def date_created(self):
> +        """See `IRockRecipeRequestBuildsJob`."""
> +        return self.context.job.date_created
> +
> +    @property
> +    def date_finished(self):
> +        """See `IRockRecipeRequestBuildsJob`."""
> +        return self.context.job.date_finished
> +
> +    @property
> +    def error_message(self):
> +        """See `IRockRecipeRequestBuildsJob`."""
> +        return self.metadata.get("error_message")
> +
> +    @error_message.setter
> +    def error_message(self, message):
> +        """See `IRockRecipeRequestBuildsJob`."""
> +        self.metadata["error_message"] = message
> +
> +    @property
> +    def build_request(self):
> +        """See `IRockRecipeRequestBuildsJob`."""
> +        return self.recipe.getBuildRequest(self.job.id)
> +
> +    def run(self):
> +        """See `IRunnableJob`."""
> +        requester = self.requester
> +        if requester is None:
> +            log.info(
> +                "Skipping %r because the requester has been deleted." % self
> +            )
> +            return
> +        try:
> +            # XXX jugmac00 2024-09-06: Implement this once we have a
> +            # RockRecipeBuild model.
> +            raise NotImplementedError

Does it make sense to be adding this job when it doesn't have all the bits that will allow it to run? Perhaps all of this could have been added in another MP?

I think it's fine to have it, but this is already such a large MP to review.

> +        except Exception as e:
> +            self.error_message = str(e)
> +            # The normal job infrastructure will abort the transaction, but
> +            # we want to commit instead: the only database changes we make
> +            # are to this job's metadata and should be preserved.
> +            transaction.commit()
> +            raise
> diff --git a/lib/lp/rocks/security.py b/lib/lp/rocks/security.py
> new file mode 100644
> index 0000000..6aee24b
> --- /dev/null
> +++ b/lib/lp/rocks/security.py
> @@ -0,0 +1,59 @@
> +# Copyright 2009-2024 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Security adapters for the rocks package."""
> +
> +__all__ = []
> +
> +from lp.app.security import AuthorizationBase, DelegatedAuthorization
> +from lp.rocks.interfaces.rockrecipe import IRockRecipe, IRockRecipeBuildRequest
> +
> +
> +class ViewRockRecipe(AuthorizationBase):
> +    """Private rock recipes are only visible to their owners and admins."""
> +
> +    permission = "launchpad.View"
> +    usedfor = IRockRecipe
> +
> +    def checkAuthenticated(self, user):
> +        return self.obj.visibleByUser(user.person)
> +
> +    def checkUnauthenticated(self):
> +        return self.obj.visibleByUser(None)
> +
> +
> +class EditRockRecipe(AuthorizationBase):
> +    permission = "launchpad.Edit"
> +    usedfor = IRockRecipe
> +
> +    def checkAuthenticated(self, user):
> +        return (
> +            user.isOwner(self.obj) or user.in_commercial_admin or user.in_admin
> +        )
> +
> +
> +class AdminRockRecipe(AuthorizationBase):
> +    """Restrict changing build settings on rock recipes.
> +
> +    The security of the non-virtualised build farm depends on these
> +    settings, so they can only be changed by "PPA"/commercial admins, or by
> +    "PPA" self admins on rock recipes that they can already edit.
> +    """
> +
> +    permission = "launchpad.Admin"
> +    usedfor = IRockRecipe
> +
> +    def checkAuthenticated(self, user):
> +        if user.in_ppa_admin or user.in_commercial_admin or user.in_admin:
> +            return True
> +        return user.in_ppa_self_admins and EditRockRecipe(
> +            self.obj
> +        ).checkAuthenticated(user)
> +
> +
> +class ViewCharmRecipeBuildRequest(DelegatedAuthorization):

class _ViewRockRecipeBuildRequest_?

> +    permission = "launchpad.View"
> +    usedfor = IRockRecipeBuildRequest
> +
> +    def __init__(self, obj):
> +        super().__init__(obj, obj.recipe, "launchpad.View")


-- 
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/472800
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/launchpad:add-basic-model-for-rock-recipe-jobs into launchpad:master.