Merge ppa-dev-tools:set-command-architecture-option into ppa-dev-tools:main

[Bryce Harrington]

Continuation of work on the ppa create/set command. This branch focuses on the addition of tests for covering --architecture's implementation, along with some assorted cleanups.

This also brings in the first user for the recently added unpack_to_dict().

9f21837... by Bryce Harrington on 2023-03-24

Fix invalid split of architectures for unittests

For regular use, architectures were being split into a list by the
create_config() routine. However, in the unittests we're skipping that
functionality and passing config directly, so the architecture string
isn't split, thus breaking the test cases.

Since it was already intended to move the architecture splitting
behavior out of create_config() to command-specific parsing, do that now
for all commands (create, set, and tests) that support an architecture
option.

cf6a842... by Bryce Harrington on 2023-03-24

Use unpack_to_dict() for splitting --architecture values

This is a bit overkill for handling architecture strings, but since
unpack_to_dict() is intended to be used for parsing all handling of
comma-separated values in the CLI, it will be most consistent if it's
used here as well.

[Athos Ribeiro (athos-ribeiro)]

LGTM!

Thanks, Bryce

review: Approve

[Bryce Harrington (bryce)]

Thanks, pushed!

Total 40 (delta 28), reused 0 (delta 0), pack-reused 0
To git+ssh://git.launchpad.net/ppa-dev-tools
   0ff0e30..4bb2310 main -> main

diff --git a/ppa/ppa.py b/ppa/ppa.py
index 671285c..aeb7854 100755
--- a/ppa/ppa.py
+++ b/ppa/ppa.py
@@ -11,6 +11,7 @@
 """A wrapper around a Launchpad Personal Package Archive object."""
 
 import re
+import sys
 
 from functools import lru_cache
 from lazr.restfulclient.errors import BadRequest, NotFound
@@ -182,7 +183,7 @@ class Ppa:
 
     @property
     @lru_cache
-    def architectures(self):
+    def architectures(self) -> list[str]:
         """Returns the architectures configured to build packages in the PPA.
 
         :rtype: list[str]
@@ -191,26 +192,30 @@ class Ppa:
         try:
             return [proc.name for proc in self.archive.processors]
         except PpaDoesNotExist as e:
-            print(e)
+            sys.stderr.write(e)
             return None
 
-    def set_architectures(self, architectures):
+    def set_architectures(self, architectures: list[str]) -> bool:
         """Configures the architectures used to build packages in the PPA.
 
         Note that some architectures may only be available upon request
         from Launchpad administrators.  ppa.constants.ARCHES_PPA is a
         list of standard architectures that don't require permissions.
 
+        :param list[str] architectures: List of processor architecture names
         :rtype: bool
-        :returns: True if architectures could be set, False on error.
+        :returns: True if architectures could be set, False on error or
+            if no architectures were specified.
         """
+        if not architectures:
+            return False
         uri_base = "https://api.launchpad.net/devel/+processors/{}"
         procs = [uri_base.format(arch) for arch in architectures]
         try:
             self.archive.setProcessors(processors=procs)
             return True
         except PpaDoesNotExist as e:
-            print(e)
+            sys.stderr.write(e)
             return False
 
     def get_binaries(self, distro=None, series=None, arch=None):
diff --git a/ppa/ppa_group.py b/ppa/ppa_group.py
index 7bccbb9..59011ea 100755
--- a/ppa/ppa_group.py
+++ b/ppa/ppa_group.py
@@ -88,7 +88,7 @@ class PpaGroup:
         """
         return self.service.people[self.name]
 
-    def create(self, ppa_name='ppa', ppa_description=None, **kwargs):
+    def create(self, ppa_name='ppa', ppa_description=None):
         """Registers a new PPA with Launchpad.
 
         If a description is not provided a default one will be generated.
@@ -104,8 +104,6 @@ class PpaGroup:
             'description': ppa_description,
             'displayname': ppa_name,
         }
-        for k in kwargs.keys():
-            ppa_settings[k] = kwargs[k]
 
         try:
             self.team.createPPA(
diff --git a/scripts/ppa b/scripts/ppa
index 164fad0..9097004 100755
--- a/scripts/ppa
+++ b/scripts/ppa
@@ -71,6 +71,7 @@ from ppa.constants import (
     ARCHES_AUTOPKGTEST,
     URL_AUTOPKGTEST,
 )
+from ppa.dict import unpack_to_dict
 from ppa.io import open_url
 from ppa.job import (
     get_waiting,
@@ -432,37 +433,8 @@ def create_config(lp: Lp, args: argparse.Namespace) -> dict[str, Any]:
     if args.dry_run:
         config['dry_run'] = True
 
-    # TODO: Each subcommand should have its own args->config parser
-    # TODO: Also, loading the values from the config file will need namespaced,
+    # TODO: Loading the values from the config file will need namespaced,
     #       so e.g. create.architectures = a,b,c
-    if args.command == 'create':
-        if args.architectures is not None:
-            if args.architectures:
-                config['architectures'] = args.architectures.split(',')
-            else:
-                warn(f"Invalid architectures '{args.architectures}'")
-                return None
-    elif args.command == 'tests':
-        if args.architectures is not None:
-            if args.architectures:
-                config['architectures'] = args.architectures.split(',')
-            else:
-                warn(f"Invalid architectures '{args.architectures}'")
-                return None
-
-        if args.releases is not None:
-            if args.releases:
-                config['releases'] = args.releases.split(',')
-            else:
-                warn(f"Invalid releases '{args.releases}'")
-                return None
-    elif args.command == "set":
-        if args.architectures is not None:
-            if args.architectures:
-                config['architectures'] = args.architectures.split(',')
-            else:
-                warn(f"Invalid architectures '{args.architectures}'")
-                return None
 
     return config
 
@@ -495,14 +467,18 @@ def command_create(lp, config):
         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()
 
     try:
         if not config.get('dry_run', False):
             ppa_group = PpaGroup(service=lp, name=team_name)
             the_ppa = ppa_group.create(ppa_name, ppa_description=description)
-            the_ppa.set_publish(config.get('publish', None))
-            the_ppa.set_architectures(architectures)
+            the_ppa.set_publish(publish)
+            if architectures:
+                the_ppa.set_architectures(architectures)
             arch_str = ', '.join(the_ppa.architectures)
         else:
             the_ppa = Ppa(ppa_name, team_name, description)
@@ -645,7 +621,10 @@ def command_set(lp, config):
         the_ppa = get_ppa(lp, config)
 
         if 'architectures' in config:
-            the_ppa.set_architectures(config['architectures'])
+            architectures = config['architectures']
+            if type(architectures) is str:
+                architectures = unpack_to_dict(architectures).keys()
+            the_ppa.set_architectures(architectures)
 
         if 'description' in config:
             the_ppa.archive.description = config['description']
@@ -806,6 +785,8 @@ def command_tests(lp, config):
         return 1
 
     architectures = config.get('architectures', ARCHES_AUTOPKGTEST)
+    if type(architectures) is str:
+        architectures = unpack_to_dict(architectures).keys()
 
     try:
         # Triggers
diff --git a/tests/helpers.py b/tests/helpers.py
index 84b36ad..2329125 100644
--- a/tests/helpers.py
+++ b/tests/helpers.py
@@ -33,15 +33,12 @@ class ArchiveMock:
         self.publish = True
         self.processors = [ProcessorMock(proc_name) for proc_name in ARCHES_PPA_DEFAULT]
 
+    def setProcessors(self, processors):
+        self.processors = [ProcessorMock(proc.split('/')[-1]) for proc in processors]
+
     def lp_save(self):
         return True
 
-    def setProcessors(self, processors):
-        self.processors = []
-        for proc in processors:
-            proc_name = proc.split('/')[-1]
-            self.processors.append(ProcessorMock(proc_name))
-
 
 class PersonMock:
     """A stand-in for a Launchpad Person object."""
diff --git a/tests/test_scripts_ppa.py b/tests/test_scripts_ppa.py
index 2ac7599..a107f92 100644
--- a/tests/test_scripts_ppa.py
+++ b/tests/test_scripts_ppa.py
@@ -146,7 +146,7 @@ def test_create_arg_parser():
         if isinstance(action, argparse._SubParsersAction)]
     subparsers = []
     for subparsers_action in subparsers_actions:
-        for choice, subparser in subparsers_action.choices.items():
+        for choice, _ in subparsers_action.choices.items():
             subparsers.append(choice)
     assert subparsers == [
         'create',
@@ -387,7 +387,6 @@ def test_create_config_from_args(command_line_options, expected_config):
     args = parser.parse_args(command_line_options)
     config = script.create_config(lp, args)
 
-    expected_result = script.DEFAULT_CONFIG
     for key, value in expected_config.items():
         assert key in config.keys()
         assert config[key] == value
@@ -451,6 +450,31 @@ def test_command_create(fake_config, monkeypatch, stdin, params, expected_ppa_co
         assert getattr(lp_ppa, key) == value
 
 
[e-mail]('architectures, expected_processors', [
+    (None, ARCHES_PPA_DEFAULT),
+    ('a', ['a']),
+    ('a,b,c', ['a', 'b', 'c']),
+    ('a, b, c', ['a', 'b', 'c']),
+    ('amd64,arm64,armhf,i386,powerpc,ppc64el,s390x',
+     ["amd64", "arm64", "armhf", "i386", "powerpc", "ppc64el", "s390x"]),
+])
+def test_command_create_with_architectures(monkeypatch, fake_config, architectures, expected_processors):
+    '''Checks that PPAs can be created with non-default architecture support.'''
+    lp = LpServiceMock()
+    config = {**fake_config, **{'architectures': architectures}}
+    monkeypatch.setattr("sys.stdin", io.StringIO('x'))
+    assert script.command_create(lp, config) == 0
+
+    # Retrieve the newly created PPA
+    team = lp.people[config['team_name']]
+    lp_ppa = team.getPPAByName(config['ppa_name'])
+
+    # Check processor architectures
+    assert lp_ppa.processors
+    assert type(lp_ppa.processors) is list
+    assert [proc.name for proc in lp_ppa.processors] == expected_processors
+
+
 @pytest.mark.xfail(reason="Unimplemented")
 def test_command_desc(fake_config):
     assert script.command_desc(fake_config) == 0
@@ -514,6 +538,35 @@ def test_command_set(fake_config, params, expected_ppa_config):
         assert getattr(lp_ppa, key) == value
 
 
[e-mail]('architectures, expected_processors', [
+    (None, ARCHES_PPA_DEFAULT),
+    ('a', ['a']),
+    ('a,b,c', ['a', 'b', 'c']),
+    ('a, b, c', ['a', 'b', 'c']),
+    ('amd64,arm64,armhf,i386,powerpc,ppc64el,s390x',
+     ["amd64", "arm64", "armhf", "i386", "powerpc", "ppc64el", "s390x"]),
+])
+def test_command_set_architectures(fake_config, architectures, expected_processors):
+    '''Checks that existing PPAs can have their architectures changed.'''
+    lp = LpServiceMock()
+
+    # Create a default PPA, for modification later
+    team = lp.people[fake_config['team_name']]
+    team.createPPA(fake_config['ppa_name'], 'x', 'y')
+
+    # Check success of the set command
+    config = {**fake_config, **{'architectures': architectures}}
+    assert script.command_set(lp, config)
+
+    # Retrieve the PPA we created earlier
+    lp_ppa = team.getPPAByName(fake_config['ppa_name'])
+
+    # Check processor architectures
+    assert lp_ppa.processors
+    assert type(lp_ppa.processors) is list
+    assert [proc.name for proc in lp_ppa.processors] == expected_processors
+
+
 @pytest.mark.xfail(reason="Unimplemented")
 def test_command_show(fake_config):
     assert script.command_show(fake_config) == 0