Re: [Launchpad-reviewers] [Merge] ~lgp171188/lpcraft:easier-way-to-add-a-ppa into lpcraft:main

[Guruprasad]

I have addressed your review comments, Andrey.
-- 
https://code.launchpad.net/~lgp171188/lpcraft/+git/lpcraft/+merge/433493
Your team Launchpad code reviewers is requested to review the proposed merge of 
~lgp171188/lpcraft:easier-way-to-add-a-ppa into lpcraft:main.


___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Re: [Launchpad-reviewers] [Merge] ~lgp171188/lpcraft:easier-way-to-add-a-ppa into lpcraft:main

[Andrey Fedoseev]

Diff comments:

> diff --git a/lpcraft/config.py b/lpcraft/config.py
> index d8d2045..f60cbc2 100644
> --- a/lpcraft/config.py
> +++ b/lpcraft/config.py
> @@ -126,12 +135,57 @@ class PackageRepository(ModelConfigDefaults):
>  """
>  
>  type: PackageType  # e.g. `apt``
> +ppa: Optional[PPAShortFormURL]  # e.g. `ppa:launchpad/ppa`
>  formats: List[PackageFormat]  # e.g. `[deb, deb-src]`
> -components: List[PackageComponent]  # e.g. `[main, universe]`
> +components: Optional[List[PackageComponent]]  # e.g. `[main, universe]`
>  suites: List[PackageSuite]  # e.g. `[bionic, focal]`
> -url: AnyHttpUrl
> +url: Optional[AnyHttpUrl]
>  trusted: Optional[bool]
>  
> +@root_validator(pre=True)
> +def validate_multiple_fields(
> +cls, values: Dict[str, Any]
> +) -> Dict[str, Any]:
> +if "url" not in values and "ppa" not in values:
> +raise ValueError(
> +"One of the following keys is required with an appropriate"
> +" value: 'url', 'ppa'."
> +)
> +elif "url" in values and "ppa" in values:
> +raise ValueError(
> +"Only one of the following keys can be specified:"
> +" 'url', 'ppa'."
> +)
> +elif "ppa" not in values and "components" not in values:
> +raise ValueError(
> +"One of the following keys is required with an appropriate"
> +" value: 'components', 'ppa'."
> +)
> +elif "ppa" in values and "components" in values:
> +raise ValueError(
> +"The 'components' key is not allowed when the 'ppa' key is"
> +" specified. PPAs only support the 'main' component."
> +)
> +return values
> +
> +@validator("components", pre=True, always=True)
> +def infer_components_if_ppa_is_set(
> +cls, v: List[PackageComponent], values: Dict[str, Any]
> +) -> List[PackageComponent]:
> +if v is None and values["ppa"]:
> +return ["main"]
> +return v
> +
> +@validator("url", pre=True, always=True)
> +def infer_url_if_ppa_is_set(
> +cls, v: AnyHttpUrl, values: Dict[str, Any]
> +) -> AnyHttpUrl:
> +if v is None and values["ppa"]:
> +return "{}/{}/ubuntu".format(
> +LAUNCHPAD_PPA_BASE_URL, values["ppa"].replace("ppa:", "")

`.replace("ppa:", "")` is probably unnecessary here

> +)
> +return v
> +
>  @validator("trusted")
>  def convert_trusted(cls, v: bool) -> str:
>  # trusted is True or False, but we need `yes` or `no`


-- 
https://code.launchpad.net/~lgp171188/lpcraft/+git/lpcraft/+merge/433493
Your team Launchpad code reviewers is requested to review the proposed merge of 
~lgp171188/lpcraft:easier-way-to-add-a-ppa into lpcraft:main.


___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Re: [Launchpad-reviewers] [Merge] ~lgp171188/lpcraft:easier-way-to-add-a-ppa into lpcraft:main

[Guruprasad]

Diff comments:

> diff --git a/lpcraft/config.py b/lpcraft/config.py
> index d8d2045..f60cbc2 100644
> --- a/lpcraft/config.py
> +++ b/lpcraft/config.py
> @@ -119,6 +121,13 @@ class PackageSuite(str, Enum):
>  jammy = "jammy"  # 22.04
>  
>  
> +class PPAShortFormURL(pydantic.ConstrainedStr):
> +"""A string with a constrained syntax to match a PPA short form URL."""
> +
> +strict = True
> +regex = 
> re.compile(r"^[a-z0-9][a-z0-9\+\._\-]+/[a-z0-9][a-z0-9\+\._\-]+$")

I checked the snapcraft parameter that Colin had referred to in the related bug 
report and there they do not use a duplicate `ppa` prefix causing the key-value 
pair to look like `ppa: ppa:launchpad/ppa`. So I updated the regex to drop the 
duplicate `ppa:` prefix but missed updating the example below. Will update it 
in the next revision while addressing review comments.

> +
> +
>  class PackageRepository(ModelConfigDefaults):
>  """A representation of a package repository.
>  
> @@ -126,12 +135,57 @@ class PackageRepository(ModelConfigDefaults):
>  """
>  
>  type: PackageType  # e.g. `apt``
> +ppa: Optional[PPAShortFormURL]  # e.g. `ppa:launchpad/ppa`

Like mentioned in the reply to Andrey's comment above, value in this example 
needs to be updated to `launchpad/ppa` so that the key-value pair in the 
configuration looks like `ppa: launchpad/ppa`.

>  formats: List[PackageFormat]  # e.g. `[deb, deb-src]`
> -components: List[PackageComponent]  # e.g. `[main, universe]`
> +components: Optional[List[PackageComponent]]  # e.g. `[main, universe]`
>  suites: List[PackageSuite]  # e.g. `[bionic, focal]`
> -url: AnyHttpUrl
> +url: Optional[AnyHttpUrl]
>  trusted: Optional[bool]
>  
> +@root_validator(pre=True)
> +def validate_multiple_fields(
> +cls, values: Dict[str, Any]
> +) -> Dict[str, Any]:
> +if "url" not in values and "ppa" not in values:
> +raise ValueError(
> +"One of the following keys is required with an appropriate"
> +" value: 'url', 'ppa'."
> +)
> +elif "url" in values and "ppa" in values:
> +raise ValueError(
> +"Only one of the following keys can be specified:"
> +" 'url', 'ppa'."
> +)
> +elif "ppa" not in values and "components" not in values:
> +raise ValueError(
> +"One of the following keys is required with an appropriate"
> +" value: 'components', 'ppa'."
> +)
> +elif "ppa" in values and "components" in values:
> +raise ValueError(
> +"The 'components' key is not allowed when the 'ppa' key is"
> +" specified. PPAs only support the 'main' component."
> +)
> +return values
> +
> +@validator("components", pre=True, always=True)
> +def infer_components_if_ppa_is_set(
> +cls, v: List[PackageComponent], values: Dict[str, Any]
> +) -> List[PackageComponent]:
> +if v is None and values["ppa"]:
> +return ["main"]
> +return v
> +
> +@validator("url", pre=True, always=True)
> +def infer_url_if_ppa_is_set(
> +cls, v: AnyHttpUrl, values: Dict[str, Any]
> +) -> AnyHttpUrl:
> +if v is None and values["ppa"]:
> +return "{}/{}/ubuntu".format(
> +LAUNCHPAD_PPA_BASE_URL, values["ppa"].replace("ppa:", "")
> +)
> +return v
> +
>  @validator("trusted")
>  def convert_trusted(cls, v: bool) -> str:
>  # trusted is True or False, but we need `yes` or `no`


-- 
https://code.launchpad.net/~lgp171188/lpcraft/+git/lpcraft/+merge/433493
Your team Launchpad code reviewers is requested to review the proposed merge of 
~lgp171188/lpcraft:easier-way-to-add-a-ppa into lpcraft:main.


___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Re: [Launchpad-reviewers] [Merge] ~lgp171188/lpcraft:easier-way-to-add-a-ppa into lpcraft:main

[Andrey Fedoseev]

Diff comments:

> diff --git a/lpcraft/config.py b/lpcraft/config.py
> index d8d2045..f60cbc2 100644
> --- a/lpcraft/config.py
> +++ b/lpcraft/config.py
> @@ -119,6 +121,13 @@ class PackageSuite(str, Enum):
>  jammy = "jammy"  # 22.04
>  
>  
> +class PPAShortFormURL(pydantic.ConstrainedStr):
> +"""A string with a constrained syntax to match a PPA short form URL."""
> +
> +strict = True
> +regex = 
> re.compile(r"^[a-z0-9][a-z0-9\+\._\-]+/[a-z0-9][a-z0-9\+\._\-]+$")

This regex doesn't match `ppa:launchpad/ppa` form, it works only for 
`launchpad/ppa`. Is that intentional? I see that you use `ppa:launchpad/ppa` as 
an example below, but don't include that form in any of the tests.

> +
> +
>  class PackageRepository(ModelConfigDefaults):
>  """A representation of a package repository.
>  


-- 
https://code.launchpad.net/~lgp171188/lpcraft/+git/lpcraft/+merge/433493
Your team Launchpad code reviewers is requested to review the proposed merge of 
~lgp171188/lpcraft:easier-way-to-add-a-ppa into lpcraft:main.


___
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

[Launchpad-reviewers] [Merge] ~lgp171188/lpcraft:easier-way-to-add-a-ppa into lpcraft:main

[Guruprasad]

Guruprasad has proposed merging ~lgp171188/lpcraft:easier-way-to-add-a-ppa into 
lpcraft:main.

Commit message:
Allow specifying PPAs using the shortform notation

LP: #1982950


Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1982950 in lpcraft: "Adding PPAs could be easier"
  https://bugs.launchpad.net/lpcraft/+bug/1982950

For more details, see:
https://code.launchpad.net/~lgp171188/lpcraft/+git/lpcraft/+merge/433493
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~lgp171188/lpcraft:easier-way-to-add-a-ppa into lpcraft:main.
diff --git a/NEWS.rst b/NEWS.rst
index 8403acb..a63549c 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -7,6 +7,8 @@ Version history
 
 - Sanitize the project name before cleaning.
 
+- Allow specifying PPAs using the shortform notation.
+
 0.0.35 (2022-10-27)
 ===
 
diff --git a/docs/configuration.rst b/docs/configuration.rst
index 258d869..d80888a 100644
--- a/docs/configuration.rst
+++ b/docs/configuration.rst
@@ -184,26 +184,34 @@ More properties can be implemented on demand.
 
 ``type`` (required)
 Specifies the type of package-repository.
-Currently only `apt` is supported.
+Currently only ``apt`` is supported.
 
 ``formats`` (required)
 Specifies the format of the package-repository.
 Supported values: ``deb`` and ``deb-src``.
 
-``components`` (required)
-Specifies the component of the package-repository,
+``components`` (optional)
+Specifies the component of the package-repository.
 One or several of ``main``, ``restricted``, ``universe``, ``multiverse``.
+This property is not allowed when the ``ppa`` key is specified. Otherwise,
+it is required.
 
 ``suites`` (required)
 Specifies the suite of the package-repository.
 One or several of ``bionic``, ``focal``, ``jammy``.
 
-``url`` (required)
+``url`` (optional)
 Specifies the URL of the package-repository,
 e.g. ``http://ppa.launchpad.net/snappy-dev/snapcraft-daily/ubuntu``.
 The URL is rendered using `Jinja2 `_.
 This can be used to supply authentication details via the *secrets*
-command line option.
+command line option. This property is not allowed when the ``ppa`` key
+is specified. Otherwise, it is required.
+
+``ppa`` (optional)
+Specifies the PPA to be used as the package repository in the short form,
+e.g. ``launchpad/ppa``. This is automatically expanded to an appropriate
+``url`` value.
 
 ``trusted`` (optional)
 Set this to ``true`` to override APT's security checks, ie accept sources
diff --git a/lpcraft/config.py b/lpcraft/config.py
index d8d2045..f60cbc2 100644
--- a/lpcraft/config.py
+++ b/lpcraft/config.py
@@ -8,13 +8,15 @@ from pathlib import Path
 from typing import Any, Dict, Iterator, List, Optional, Type, Union
 
 import pydantic
-from pydantic import AnyHttpUrl, StrictStr, validator
+from pydantic import AnyHttpUrl, StrictStr, root_validator, validator
 
 from lpcraft.errors import ConfigurationError
 from lpcraft.plugins import PLUGINS
 from lpcraft.plugins.plugins import BaseConfig, BasePlugin
 from lpcraft.utils import load_yaml
 
+LAUNCHPAD_PPA_BASE_URL = "https://ppa.launchpadcontent.net;
+
 
 class _Identifier(pydantic.ConstrainedStr):
 """A string with constrained syntax used as a short identifier.
@@ -119,6 +121,13 @@ class PackageSuite(str, Enum):
 jammy = "jammy"  # 22.04
 
 
+class PPAShortFormURL(pydantic.ConstrainedStr):
+"""A string with a constrained syntax to match a PPA short form URL."""
+
+strict = True
+regex = re.compile(r"^[a-z0-9][a-z0-9\+\._\-]+/[a-z0-9][a-z0-9\+\._\-]+$")
+
+
 class PackageRepository(ModelConfigDefaults):
 """A representation of a package repository.
 
@@ -126,12 +135,57 @@ class PackageRepository(ModelConfigDefaults):
 """
 
 type: PackageType  # e.g. `apt``
+ppa: Optional[PPAShortFormURL]  # e.g. `ppa:launchpad/ppa`
 formats: List[PackageFormat]  # e.g. `[deb, deb-src]`
-components: List[PackageComponent]  # e.g. `[main, universe]`
+components: Optional[List[PackageComponent]]  # e.g. `[main, universe]`
 suites: List[PackageSuite]  # e.g. `[bionic, focal]`
-url: AnyHttpUrl
+url: Optional[AnyHttpUrl]
 trusted: Optional[bool]
 
+@root_validator(pre=True)
+def validate_multiple_fields(
+cls, values: Dict[str, Any]
+) -> Dict[str, Any]:
+if "url" not in values and "ppa" not in values:
+raise ValueError(
+"One of the following keys is required with an appropriate"
+" value: 'url', 'ppa'."
+)
+elif "url" in values and "ppa" in values:
+raise ValueError(
+"Only one of the following keys can be specified:"
+" 'url', 'ppa'."
+)
+elif "ppa" not in values and "components" not in values:
+raise ValueError(
+"One of