Merge ~cjwatson/launchpad:generic-artifactory-properties into launchpad:master

[Colin Watson]

Add generic.name, generic.version, and soss.type Artifactory properties

For most artifact types, Artifactory sets "name" and "version" properties itself by scanning the artifact. However, for generic artifacts it obviously can't do this, since the content doesn't necessarily have any particular known structure. Since we already require generic artifacts to have "name" and "version" output properties set by the build job, fill in this gap by passing those on to Artifactory.

`soss.type` makes it easier for automation built on top of Artifactory to tell what any given artifact is meant to be, without having to look at the `launchpad.*` properties which we consider to be implementation details.

[Jürgen Gmach (jugmac00)]

LGTM

I think it would be great to have some description of the properties in the docstring at least. Currently, it feels a bit that like we set some properties somehow and somewhere, but for unknown reasons.

I think it makes no sense to repeat the logic of the source code, but somehow give some spec, e.g. what properties are expected.

review: Approve

01a8cbb... by Colin Watson on 2022-12-14

Add some more commentary on additional properties

[Colin Watson (cjwatson)]

There's already a comment a bit further up (outside the patch context) linking to the internal spreadsheet with the property schema we intend to expose. I've added a bit more commentary now though, as this might indeed be a bit obscure if you haven't been in all the discussions ...

[Jürgen Gmach (jugmac00)]

Thank you!

diff --git a/lib/lp/archivepublisher/artifactory.py b/lib/lp/archivepublisher/artifactory.py
index 9fc4fc7..2d16d99 100644
--- a/lib/lp/archivepublisher/artifactory.py
+++ b/lib/lp/archivepublisher/artifactory.py
@@ -22,9 +22,10 @@ from packaging import utils as packaging_utils
 from zope.security.proxy import isinstance as zope_isinstance
 
 from lp.archivepublisher.diskpool import FileAddActionEnum, poolify
+from lp.registry.interfaces.sourcepackage import SourcePackageFileType
 from lp.services.config import config
 from lp.services.librarian.utils import copy_and_close
-from lp.soyuz.enums import ArchiveRepositoryFormat
+from lp.soyuz.enums import ArchiveRepositoryFormat, BinaryPackageFileType
 from lp.soyuz.interfaces.archive import IArchive
 from lp.soyuz.interfaces.files import (
     IBinaryPackageFile,
@@ -205,6 +206,10 @@ class ArtifactoryPoolEntry:
         the AQL search via `ArtifactoryPool.getAllArtifacts`, and so
         `updateProperties` will always try to update them even if there
         aren't really any changes.
+
+        We also set an assortment of additional metadata items, specified in
+        https://docs.google.com/spreadsheets/d/15Xkdi-CRu2NiQfLoclP5PKW63Zw6syiuao8VJG7zxvw
+        (private).
         """
         properties = {}
         properties["launchpad.release-id"] = [release_id]
@@ -271,8 +276,14 @@ class ArtifactoryPoolEntry:
         # (private).
         if ISourcePackageReleaseFile.providedBy(self.pub_file):
             release = self.pub_file.sourcepackagerelease
+            # Allow automation built on top of Artifactory to tell that this
+            # is a source package.
+            properties["soss.type"] = ["source"]
         elif IBinaryPackageFile.providedBy(self.pub_file):
             release = self.pub_file.binarypackagerelease
+            # Allow automation built on top of Artifactory to tell that this
+            # is a binary package.
+            properties["soss.type"] = ["binary"]
         else:
             # There are no other kinds of `IPackageReleaseFile` at the moment.
             raise AssertionError("Unsupported file: %r" % self.pub_file)
@@ -298,6 +309,22 @@ class ArtifactoryPoolEntry:
                     # likely that a path will begin with "spdx:" so there
                     # probably won't be significant confusion in practice.
                     properties["soss.license"] = [license_field["path"]]
+        if self.pub_file.filetype in (
+            SourcePackageFileType.GENERIC,
+            BinaryPackageFileType.GENERIC,
+        ):
+            # For most artifact types, Artifactory sets "name" and "version"
+            # properties itself by scanning the artifact.  However, for
+            # generic artifacts it obviously can't do this, since the
+            # content doesn't necessarily have any particular known
+            # structure.  Since we already require generic artifacts to have
+            # "name" and "version" output properties set by the build job,
+            # fill in this gap by passing those on to Artifactory.
+            package_name = release.getUserDefinedField("name")
+            if package_name is None:
+                package_name = self.source_name
+            properties["generic.name"] = [package_name]
+            properties["generic.version"] = [self.source_version]
         return properties
 
     def addFile(self):
diff --git a/lib/lp/archivepublisher/tests/test_artifactory.py b/lib/lp/archivepublisher/tests/test_artifactory.py
index 4179c2e..da1a31a 100644
--- a/lib/lp/archivepublisher/tests/test_artifactory.py
+++ b/lib/lp/archivepublisher/tests/test_artifactory.py
@@ -192,6 +192,7 @@ class TestArtifactoryPool(TestCase):
                 "launchpad.release-id": ["binary:1"],
                 "launchpad.source-name": ["foo"],
                 "launchpad.source-version": ["1.0+1"],
+                "soss.type": ["binary"],
             },
             foo.getProperties(),
         )
@@ -317,11 +318,13 @@ class TestArtifactoryPool(TestCase):
                     "launchpad.release-id": ["binary:1"],
                     "launchpad.source-name": ["foo"],
                     "launchpad.source-version": ["1.0"],
+                    "soss.type": ["binary"],
                 },
                 PurePath("pool/f/foo/foo-1.1.deb"): {
                     "launchpad.release-id": ["binary:2"],
                     "launchpad.source-name": ["foo"],
                     "launchpad.source-version": ["1.1"],
+                    "soss.type": ["binary"],
                 },
             },
             pool.getAllArtifacts(
@@ -345,6 +348,7 @@ class TestArtifactoryPool(TestCase):
                     "launchpad.release-id": ["binary:3"],
                     "launchpad.source-name": ["bar"],
                     "launchpad.source-version": ["1.0"],
+                    "soss.type": ["binary"],
                 },
             },
             pool.getAllArtifacts(
@@ -369,6 +373,7 @@ class TestArtifactoryPool(TestCase):
                     "launchpad.release-id": ["binary:4"],
                     "launchpad.source-name": ["qux"],
                     "launchpad.source-version": ["1.0"],
+                    "soss.type": ["binary"],
                 },
             },
             pool.getAllArtifacts(
@@ -393,6 +398,7 @@ class TestArtifactoryPool(TestCase):
                     "launchpad.release-id": ["source:5"],
                     "launchpad.source-name": ["go-module"],
                     "launchpad.source-version": ["v0.0.1"],
+                    "soss.type": ["source"],
                 },
             },
             pool.getAllArtifacts(
@@ -409,6 +415,7 @@ class TestArtifactoryPool(TestCase):
             filename="bar-1.0.tar.gz",
             release_type=FakeReleaseType.SOURCE,
             release_id=1,
+            filetype=SourcePackageFileType.GENERIC,
             user_defined_fields=[
                 ("name", "bar"),
                 ("version", "1.0"),
@@ -418,9 +425,12 @@ class TestArtifactoryPool(TestCase):
         self.assertEqual(
             {
                 PurePath("bar/1.0/bar-1.0.tar.gz"): {
+                    "generic.name": ["bar"],
+                    "generic.version": ["1.0"],
                     "launchpad.release-id": ["source:1"],
                     "launchpad.source-name": ["bar"],
                     "launchpad.source-version": ["1.0"],
+                    "soss.type": ["source"],
                 },
             },
             pool.getAllArtifacts(
@@ -453,6 +463,7 @@ class TestArtifactoryPool(TestCase):
                     "launchpad.source-name": ["bar"],
                     "launchpad.source-version": ["1.0"],
                     "pypi.requires.python": [""],
+                    "soss.type": ["binary"],
                 },
             },
             pool.getAllArtifacts(
@@ -521,6 +532,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                 "launchpad.release-id": ["source:%d" % spr.id],
                 "launchpad.source-name": ["foo"],
                 "launchpad.source-version": ["1.0"],
+                "soss.type": ["source"],
             },
             path.properties,
         )
@@ -535,6 +547,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                 "deb.name": [spr.name],
                 "deb.version": [spr.version],
                 "soss.license": ["debian/copyright"],
+                "soss.type": ["source"],
             },
             path.properties,
         )
@@ -594,6 +607,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                 "launchpad.release-id": ["binary:%d" % bpr.id],
                 "launchpad.source-name": ["foo"],
                 "launchpad.source-version": ["1.0"],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -609,6 +623,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                 "deb.component": ["main"],
                 "deb.architecture": [processor.name],
                 "soss.license": ["/usr/share/doc/foo/copyright"],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -662,6 +677,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                 "launchpad.release-id": ["binary:%d" % bpr.id],
                 "launchpad.source-name": ["foo"],
                 "launchpad.source-version": ["1.0"],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -679,6 +695,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     sorted(das.architecturetag for das in dases)
                 ),
                 "soss.license": ["/usr/share/doc/foo/copyright"],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -738,6 +755,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["source"],
             },
             path.properties,
         )
@@ -754,6 +772,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["source"],
             },
             path.properties,
         )
@@ -814,6 +833,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["source"],
             },
             path.properties,
         )
@@ -830,6 +850,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["source"],
             },
             path.properties,
         )
@@ -889,6 +910,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["source"],
             },
             path.properties,
         )
@@ -905,6 +927,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["source"],
             },
             path.properties,
         )
@@ -981,6 +1004,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -997,6 +1021,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -1064,6 +1089,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -1080,6 +1106,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -1147,6 +1174,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -1163,6 +1191,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -1219,6 +1248,8 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
         self.assertFalse(path.is_symlink())
         self.assertEqual(
             {
+                "generic.name": ["foo"],
+                "generic.version": ["1.0"],
                 "launchpad.release-id": ["source:%d" % spr.id],
                 "launchpad.source-name": ["foo-package"],
                 "launchpad.source-version": ["1.0"],
@@ -1226,12 +1257,15 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["source"],
             },
             path.properties,
         )
         pool.updateProperties(spr.name, spr.version, [sprf], spphs)
         self.assertEqual(
             {
+                "generic.name": ["foo"],
+                "generic.version": ["1.0"],
                 "launchpad.release-id": ["source:%d" % spr.id],
                 "launchpad.source-name": ["foo-package"],
                 "launchpad.source-version": ["1.0"],
@@ -1242,6 +1276,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["source"],
             },
             path.properties,
         )
@@ -1302,6 +1337,8 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
         self.assertFalse(path.is_symlink())
         self.assertEqual(
             {
+                "generic.name": ["foo"],
+                "generic.version": ["1.0"],
                 "launchpad.release-id": ["binary:%d" % bpr.id],
                 "launchpad.source-name": ["foo"],
                 "launchpad.source-version": ["1.0"],
@@ -1309,12 +1346,15 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
         pool.updateProperties(bpph.pool_name, bpph.pool_version, [bpf], bpphs)
         self.assertEqual(
             {
+                "generic.name": ["foo"],
+                "generic.version": ["1.0"],
                 "launchpad.release-id": ["binary:%d" % bpr.id],
                 "launchpad.source-name": ["foo"],
                 "launchpad.source-version": ["1.0"],
@@ -1325,6 +1365,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                     ci_build.git_repository.getCodebrowseUrl()
                 ],
                 "soss.commit_id": [ci_build.commit_sha1],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -1377,6 +1418,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                 "launchpad.source-name": ["foo"],
                 "launchpad.source-version": ["1.0"],
                 "deb.version": ["1.0"],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
@@ -1393,6 +1435,7 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
                 "deb.architecture": [das.architecturetag],
                 "deb.version": ["1.0"],
                 "soss.license": ["/usr/share/doc/foo/copyright"],
+                "soss.type": ["binary"],
             },
             path.properties,
         )
diff --git a/lib/lp/archivepublisher/tests/test_pool.py b/lib/lp/archivepublisher/tests/test_pool.py
index a4d4cf5..6e515ad 100644
--- a/lib/lp/archivepublisher/tests/test_pool.py
+++ b/lib/lp/archivepublisher/tests/test_pool.py
@@ -10,8 +10,9 @@ from lazr.enum import EnumeratedType, Item
 from zope.interface import alsoProvides, implementer
 
 from lp.archivepublisher.diskpool import DiskPool, poolify, unpoolify
+from lp.registry.interfaces.sourcepackage import SourcePackageFileType
 from lp.services.log.logger import BufferLogger
-from lp.soyuz.enums import ArchiveRepositoryFormat
+from lp.soyuz.enums import ArchiveRepositoryFormat, BinaryPackageFileType
 from lp.soyuz.interfaces.files import (
     IBinaryPackageFile,
     IPackageReleaseFile,
@@ -78,11 +79,13 @@ class FakePackageReleaseFile:
         filename,
         release_type=FakeReleaseType.BINARY,
         release_id=1,
+        filetype=None,
         user_defined_fields=None,
         ci_build=None,
     ):
         self.libraryfile = FakeLibraryFileAlias(contents, filename)
         if release_type == FakeReleaseType.SOURCE:
+            self.filetype = filetype or SourcePackageFileType.DSC
             self.sourcepackagereleaseID = release_id
             self.sourcepackagerelease = FakePackageRelease(
                 release_id,
@@ -91,6 +94,7 @@ class FakePackageReleaseFile:
             )
             alsoProvides(self, ISourcePackageReleaseFile)
         elif release_type == FakeReleaseType.BINARY:
+            self.filetype = filetype or BinaryPackageFileType.DEB
             self.binarypackagereleaseID = release_id
             self.binarypackagerelease = FakePackageRelease(
                 release_id, user_defined_fields=user_defined_fields
@@ -107,6 +111,7 @@ class PoolTestingFile:
         filename,
         release_type=FakeReleaseType.BINARY,
         release_id=1,
+        filetype=None,
         user_defined_fields=None,
     ):
         self.pool = pool
@@ -117,6 +122,7 @@ class PoolTestingFile:
             filename,
             release_type=release_type,
             release_id=release_id,
+            filetype=filetype,
             user_defined_fields=user_defined_fields,
         )
 
diff --git a/lib/lp/archivepublisher/tests/test_publisher.py b/lib/lp/archivepublisher/tests/test_publisher.py
index e52d5a3..44f54c3 100644
--- a/lib/lp/archivepublisher/tests/test_publisher.py
+++ b/lib/lp/archivepublisher/tests/test_publisher.py
@@ -4221,6 +4221,7 @@ class TestArtifactoryPublishing(TestPublisherBase):
                 "launchpad.source-name": ["hello"],
                 "launchpad.source-version": ["1.0"],
                 "soss.license": ["debian/copyright"],
+                "soss.type": ["source"],
             },
             source_path.properties,
         )
@@ -4241,6 +4242,7 @@ class TestArtifactoryPublishing(TestPublisherBase):
                 "launchpad.source-name": ["hello"],
                 "launchpad.source-version": ["1.0"],
                 "soss.license": ["/usr/share/doc/hello/copyright"],
+                "soss.type": ["binary"],
             },
             binary_path.properties,
         )
@@ -4318,6 +4320,7 @@ class TestArtifactoryPublishing(TestPublisherBase):
                 "launchpad.source-name": ["hello"],
                 "launchpad.source-version": ["1.0"],
                 "soss.license": ["debian/copyright"],
+                "soss.type": ["source"],
             },
             dsc_path.properties,
         )
@@ -4333,6 +4336,7 @@ class TestArtifactoryPublishing(TestPublisherBase):
                 "launchpad.source-name": ["hello"],
                 "launchpad.source-version": ["1.0"],
                 "soss.license": ["debian/copyright"],
+                "soss.type": ["source"],
             },
             tar_path.properties,
         )
@@ -4347,6 +4351,7 @@ class TestArtifactoryPublishing(TestPublisherBase):
                 "launchpad.source-name": ["hello"],
                 "launchpad.source-version": ["1.0"],
                 "soss.license": ["/usr/share/doc/hello/copyright"],
+                "soss.type": ["binary"],
             },
             binary_path.properties,
         )
@@ -4408,6 +4413,7 @@ class TestArtifactoryPublishing(TestPublisherBase):
                 "launchpad.source-name": ["hello"],
                 "launchpad.source-version": ["1.0"],
                 "soss.license": ["debian/copyright"],
+                "soss.type": ["source"],
             },
             source_path.properties,
         )
@@ -4419,6 +4425,7 @@ class TestArtifactoryPublishing(TestPublisherBase):
                 "launchpad.source-name": ["hello"],
                 "launchpad.source-version": ["1.0"],
                 "soss.license": ["/usr/share/doc/hello/copyright"],
+                "soss.type": ["binary"],
             },
             binary_path.properties,
         )