Merge ppa-dev-tools:set-command-private-ppa into ppa-dev-tools:main

[Bryce Harrington]

Implements support for creating private PPAs and toggling PPAs public vs. private.

Creating private PPAs requires special authorization that I don't seem to have, with the result that I've not been able to verify that this actually works correctly "live". However, I've included checks for some of the authorization errors I've hit, and implemented a slew of unit tests. I'll try to find someone with access to do the live verification, but apart from that I think this code is ready for review and landing.

As usual, to run the testsuite, do:

    $ make check

or

    $ pytest-3

And the smoketest has been updated to include toggling --private:

    $ ./tests/smoketest_ppa_set

However as mentioned, unless you have authorization to modify privacy on PPAs that will merely generate errors.

[Lena Voytek (lvoytek)]

LGTM! just one note for one of the help strings

review: Approve

[Bryce Harrington (bryce)]

Thanks Lena, I've squashed that change in, updated this branch, and landed to main.

stirling: ~/src/PpaDevTools/ppa-dev-tools-commands-set$ git merge --ff-only set-command-private-ppa
Updating 4fa781a..0f7a075
Fast-forward
 ppa/ppa.py | 24 ++++++++++++++++++++++++
 ppa/ppa_group.py | 4 +++-
 scripts/ppa | 64 +++++++++++++++++++++++++++++++++++++++++++---------------------
 tests/helpers.py | 7 +++++--
 tests/smoketest_ppa_set | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/test_ppa_group.py | 10 ++++++++++
 tests/test_scripts_ppa.py | 25 ++++++++++++++++++-------
 7 files changed, 173 insertions(+), 31 deletions(-)
 create mode 100755 tests/smoketest_ppa_set
stirling: ~/src/PpaDevTools/ppa-dev-tools-commands-set$ git push
Enumerating objects: 57, done.
Counting objects: 100% (57/57), done.
Delta compression using up to 12 threads
Compressing objects: 100% (40/40), done.
Writing objects: 100% (47/47), 6.94 KiB | 6.94 MiB/s, done.
Total 47 (delta 29), reused 0 (delta 0), pack-reused 0
To git+ssh://git.launchpad.net/ppa-dev-tools
   4fa781a..0f7a075 main -> main

diff --git a/ppa/ppa.py b/ppa/ppa.py
index 2dd46eb..c4ce1fe 100755
--- a/ppa/ppa.py
+++ b/ppa/ppa.py
@@ -198,6 +198,30 @@ class Ppa:
 
     @property
     @lru_cache
+    def is_private(self) -> bool:
+        """Indicates if the PPA is private or public.
+
+        :rtype: bool
+        :returns: True if the archive is private, False if public.
+        """
+        return self.archive.private
+
+    def set_private(self, private: bool):
+        """Attempts to configure the PPA as private.
+
+        Note that PPAs can't be changed to private if they ever had any
+        sources published, or if the owning person or team is not
+        permitted to hold private PPAs.
+
+        :param bool private: Whether the PPA should be private or public.
+        """
+        if private is None:
+            return
+        self.archive.private = private
+        self.archive.lp_save()
+
+    @property
+    @lru_cache
     def publish(self):
         return self.archive.publish
 
diff --git a/ppa/ppa_group.py b/ppa/ppa_group.py
index c9747a5..3061100 100755
--- a/ppa/ppa_group.py
+++ b/ppa/ppa_group.py
@@ -88,13 +88,14 @@ class PpaGroup:
         """
         return self.service.people[self.name]
 
-    def create(self, ppa_name='ppa', ppa_description=None):
+    def create(self, ppa_name='ppa', ppa_description=None, private=False):
         """Registers a new PPA with Launchpad.
 
         If a description is not provided a default one will be generated.
 
         :param str ppa_name: Name for the PPA to create.
         :param str ppa_description: Text description of the PPA.
+        :param bool private: Limit access to PPA to only subscribed users and owner.
         :rtype: Ppa
         :returns: A Ppa object that describes the created PPA.
 
@@ -103,6 +104,7 @@ class PpaGroup:
         ppa_settings = {
             'description': ppa_description,
             'displayname': ppa_name,
+            'private': private
         }
 
         try:
diff --git a/scripts/ppa b/scripts/ppa
index 67dec11..ea624e0 100755
--- a/scripts/ppa
+++ b/scripts/ppa
@@ -55,7 +55,7 @@ from inspect import currentframe
 from textwrap import indent
 from typing import Any
 from distro_info import UbuntuDistroInfo
-from lazr.restfulclient.errors import BadRequest
+from lazr.restfulclient.errors import BadRequest, Unauthorized
 
 try:
     from ruamel import yaml
@@ -193,33 +193,37 @@ def add_basic_config_options(parser: argparse.ArgumentParser) -> None:
         help="A short description of the archive.  URLs will be rendered as links.  (See also 'ppa desc'.)"
     )
 
-    # Enable/Disable uploading
+    # Dependencies
     parser.add_argument(
-        '--enable',
-        dest="set_enabled",
-        action='store_true',
-        help="Accept and build packages uploaded to the PPA."
+        '--ppa-dependencies', '--ppa-depends',
+        dest="ppa_dependencies", action='store',
+        help="The set of other PPAs this PPA should use for satisfying build dependencies."
     )
+
+    # Public/Private access
     parser.add_argument(
-        '--disable',
-        dest="set_disabled",
+        '--private', '-P',
+        dest="private",
         action='store_true',
-        help="Do not accept or build packages uploaded to the PPA."
+        help=(
+            "Restrict access to the PPA to its owner and subscribers. " +
+            "This can only be changed if the archive has never had any " +
+            "sources published and the owner/group has permission to do so."
+        )
     )
-
-    # Dependencies
     parser.add_argument(
-        '--ppa-dependencies', '--ppa-depends',
-        dest="ppa_dependencies", action='store',
-        help="The set of other PPAs this PPA should use for satisfying build dependencies."
+        '--public',
+        dest="private",
+        action='store_false',
+        help="Allow access to the PPA to anyone."
     )
 
+    # Publishing
     parser.add_argument(
         '--publish',
         dest="publish", action='store_true',
         help=("Allow built packages in the PPA to be made available for download.")
     )
-
     parser.add_argument(
         '--no-publish',
         dest="publish", action='store_false',
@@ -304,7 +308,7 @@ def create_arg_parser() -> argparse.ArgumentParser:
     set_parser = subparser.add_parser(
         'set',
         argument_default=argparse.SUPPRESS,
-        help='set help',
+        help='Applies one or more configuration changes to the PPA.',
         prog=progname,
     )
     add_global_options(set_parser)
@@ -429,7 +433,7 @@ def create_config(lp: Lp, args: argparse.Namespace) -> dict[str, Any]:
         config = dict(DEFAULT_CONFIG)
 
     # Map all non-empty elements from argparse Namespace into config dict
-    config.update({k: v for k, v in vars(args).items() if v})
+    config.update({k: v for k, v in vars(args).items() if v is not None})
 
     # Use defaults for any remaining parameters not yet configured
     for k, v in DEFAULT_CONFIG.items():
@@ -498,8 +502,6 @@ def command_create(lp: Lp, config: dict[str, str]) -> int:
         warn("Could not determine owning person or team LP username")
         return os.EX_USAGE
 
-    publish = config.get('publish', None)
-
     architectures = config.get('architectures', ARCHES_PPA_ALL)
     if type(architectures) is str:
         architectures = unpack_to_dict(architectures).keys()
@@ -507,8 +509,12 @@ def command_create(lp: Lp, config: dict[str, str]) -> int:
     try:
         if not config.get('dry_run', False):
             ppa_group = PpaGroup(service=lp, name=owner_name)
-            the_ppa = ppa_group.create(ppa_name, ppa_description=description)
-            the_ppa.set_publish(publish)
+            the_ppa = ppa_group.create(
+                ppa_name,
+                ppa_description=description,
+                private=config.get('private', False)
+                )
+            the_ppa.set_publish(config.get('publish', None))
             if architectures:
                 the_ppa.set_architectures(architectures)
             arch_str = ', '.join(the_ppa.architectures)
@@ -534,6 +540,12 @@ def command_create(lp: Lp, config: dict[str, str]) -> int:
             print("  sudo add-apt-repository -yus {}".format(the_ppa.address))
             print("  sudo apt-get install <package(s)>")
         return os.EX_OK
+    except Unauthorized as e:
+        error(f"Insufficient authorization to create '{ppa_name}' under ownership of '{owner_name}'.")
+        return os.EX_NOPERM
+    except KeyError as e:
+        error(f"No such person or team '{owner_name}'")
+        return os.EX_NOUSER
     except PpaAlreadyExists as e:
         warn(o2str(e.message))
         return 3
@@ -678,9 +690,18 @@ def command_set(lp: Lp, config: dict[str, str]) -> int:
         if 'publish' in config:
             the_ppa.archive.publish = config.get('publish')
 
+        the_ppa.set_private(config.get('private', None))
+
         return the_ppa.archive.lp_save()
+    except Unauthorized as e:
+        if b'private' in e.content:
+            error(f"Insufficient authorization to change privacy for PPA '{the_ppa.name}'.")
+        else:
+            error(f"Insufficient authorization to modify PPA '{the_ppa.name}'.")
+        return os.EX_NOPERM
     except PpaDoesNotExist as e:
         print(e)
+        return 1
     except ValueError as e:
         print(f"Error: {e}")
         return os.EX_USAGE
@@ -819,6 +840,7 @@ def command_wait(lp: Lp, config: dict[str, str]) -> int:
         return os.EX_OK
     except PpaDoesNotExist as e:
         print(e)
+        return 1
     except ValueError as e:
         print(f"Error: {e}")
         return os.EX_USAGE
diff --git a/tests/helpers.py b/tests/helpers.py
index c607d3c..99d5e38 100644
--- a/tests/helpers.py
+++ b/tests/helpers.py
@@ -44,8 +44,9 @@ class ArchiveMock:
         self.displayname = name
         self.description = description
         self.owner = owner
-        self.publish = True
+        self.private = False
         self.processors = [ProcessorMock(proc_name) for proc_name in ARCHES_PPA_DEFAULT]
+        self.publish = True
         self.published_sources = []
 
     def setProcessors(self, processors):
@@ -64,12 +65,14 @@ class PersonMock:
         self.name = name
         self._ppas = []
 
-    def createPPA(self, name, description, displayname):
+    def createPPA(self, name, description, displayname, private=None):
         for ppa in self._ppas:
             if ppa.name == name:
                 raise PpaAlreadyExists(name)
         ppa = Ppa(name, self.name, description)
         Ppa.archive = ArchiveMock(ppa.name, ppa.description, self)
+        if isinstance(private, bool):
+            Ppa.archive.private = private
         self._ppas.append(ppa)
         return True
 
diff --git a/tests/smoketest_ppa_set b/tests/smoketest_ppa_set
new file mode 100755
index 0000000..7724d27
--- /dev/null
+++ b/tests/smoketest_ppa_set
@@ -0,0 +1,70 @@
+#!/bin/bash
+
+testsdir=$(dirname "$(readlink -f "${0}")")
+progdir="${testsdir}/../scripts"
+script="${progdir}/ppa"
+ppa_name="ppa-test-set-${RANDOM}"
+
+[ -e "${script}" ] || {
+    echo "Could not locate ppa command '${script}'."
+    exit 1
+}
+
+ppa_create() {
+    echo -n "* Creating ${ppa_name} .."
+    if "${script}" create "${ppa_name}"; then
+        echo ". OK"
+    else
+        echo "! ERROR"
+        exit 1
+    fi
+}
+
+ppa_destroy() {
+    echo -n "* Destroying ${ppa_name} .."
+    if "${script}" destroy "${ppa_name}"; then
+        echo ". OK"
+    else
+        echo "! ERROR"
+        exit 1
+    fi
+}
+
+ppa_status() {
+    "${script}" status "${ppa_name}"
+}
+
+ppa_set() {
+    echo -n "* Setting ${1} .."
+    if "${script}" set "${ppa_name}" "${@}"; then
+        echo ". OK"
+    else
+        echo "! ERROR"
+    fi
+}
+
+echo "### Default configuration ###"
+ppa_create && \
+    trap "ppa_destroy" EXIT
+ppa_status
+
+echo
+echo "### Set parameters ###"
+ppa_set --displayname "parameters-set"
+ppa_set --description "Updated PPA with various settings changed"
+ppa_set --all-architectures
+ppa_set --no-publish
+ppa_status
+
+echo
+echo "### Restore to default parameters ###"
+ppa_set --displayname "${ppa_name}"
+ppa_set --description ""
+ppa_set --default-architectures
+ppa_set --publish
+ppa_status
+
+echo
+echo "### Private PPA ###"
+ppa_set --private
+ppa_set --public
diff --git a/tests/test_ppa_group.py b/tests/test_ppa_group.py
index f53799f..6539c3a 100644
--- a/tests/test_ppa_group.py
+++ b/tests/test_ppa_group.py
@@ -69,6 +69,16 @@ def test_create_with_owner():
     assert ppa.address == 'ppa:test_owner_name/ppa_test_name'
 
 
+def test_create_private():
+    """Check creating a private PPA."""
+    lp = LpServiceMock()
+    ppa_group = PpaGroup(service=lp, name='me')
+    ppa = ppa_group.create('private_ppa', private=True)
+    assert ppa is not None
+    assert ppa.address == 'ppa:me/private_ppa'
+    assert ppa.is_private is True
+
+
 def test_list_ppas():
     """Check listing the PPAs for a PPA group."""
     test_ppa_list = ['a', 'b', 'c', 'd']
diff --git a/tests/test_scripts_ppa.py b/tests/test_scripts_ppa.py
index f82a9e4..93988d0 100644
--- a/tests/test_scripts_ppa.py
+++ b/tests/test_scripts_ppa.py
@@ -232,13 +232,16 @@ def test_create_arg_parser_basic_config(command):
     assert args.description == 'x'
     args.description = None
 
-    # Check --enable / --disable
-    args = parser.parse_args([command, 'test-ppa', '--enable'])
-    assert args.set_enabled is True
-    args.set_enabled = False
-    args = parser.parse_args([command, 'test-ppa', '--disable'])
-    assert args.set_disabled is True
-    args.set_disabled = False
+    # Check --public / -P|--private
+    args = parser.parse_args([command, 'test-ppa', '--public'])
+    assert args.private is False
+    args.public = None
+    args = parser.parse_args([command, 'test-ppa', '--private'])
+    assert args.private is True
+    args.private = None
+    args = parser.parse_args([command, 'test-ppa', '-P'])
+    assert args.private is True
+    args.private = None
 
     # Check --ppa-dependencies <PPA[,...]>
     args = parser.parse_args([command, 'test-ppa', '--ppa-dependencies', 'a,b,c'])
@@ -431,6 +434,8 @@ def test_create_arg_parser_tests():
     (['create', 'aa/bb'], {'command': 'create', 'owner_name': 'aa', 'ppa_name': 'bb'}),
     (['create', 'bb', '--owner', 'aa'], {'command': 'create', 'owner_name': 'aa', 'ppa_name': 'bb'}),
     (['create', 'bb', '--owner-name', 'aa'], {'command': 'create', 'owner_name': 'aa', 'ppa_name': 'bb'}),
+    (['create', 'bb', '--private'], {'command': 'create', 'private': True}),
+    (['create', 'bb', '--public'], {'command': 'create', 'private': False}),
     (['create', 'bb', '--team', 'aa'], {'command': 'create', 'owner_name': 'aa', 'ppa_name': 'bb'}),
     (['create', 'bb', '--team-name', 'aa'], {'command': 'create', 'owner_name': 'aa', 'ppa_name': 'bb'}),
     ])
@@ -489,6 +494,8 @@ def test_create_config_from_args_error(args, expected_exception):
     (None, {'ppa_name': 'a'}, {'displayname': 'a'}),
     (None, {'publish': True}, {'publish': True}),
     (None, {'publish': False}, {'publish': False}),
+    (None, {'private': True}, {'private': True}),
+    (None, {'private': False}, {'private': False}),
 ])
 def test_command_create(fake_config, monkeypatch, stdin, params, expected_ppa_config):
     '''Checks create command produces a PPA with expected configuration.'''
@@ -603,6 +610,10 @@ def test_command_not_exists(fake_config):
     ({}, {'publish': True}),
     ({'publish': False}, {'publish': False}),
     ({'publish': True}, {'publish': True}),
+
+    ({}, {'private': False}),
+    ({'private': False}, {'private': False}),
+    ({'private': True}, {'private': True}),
 ])
 def test_command_set(fake_config, params, expected_ppa_config):
     '''Checks that the set command properly requests PPA configuration changes.'''