launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28351
[Merge] ~cjwatson/launchpad:fix-artifactory-pool into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:fix-artifactory-pool into launchpad:master.
Commit message:
Fix some details of Artifactory integration
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/419731
Testing against a real Artifactory instance revealed some bugs:
* The `artifactory` module doesn't seem to pass through `ArtifactoryPath.auth` in all circumstances; setting authentication information on the session appears to be both necessary and sufficient in practice.
* `config.launchpad.https_proxy` doesn't exist, and was a typo for `config.launchpad.http_proxy`.
* Artifactory sets several properties by itself when scanning uploaded artifacts, and we shouldn't remove those.
* multi-value properties come back from AQL queries in a way that we weren't expecting; the simplest way to handle this seems to be to consistently handle property values as lists, even if the property only has a single value.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-artifactory-pool into launchpad:master.
diff --git a/lib/lp/archivepublisher/artifactory.py b/lib/lp/archivepublisher/artifactory.py
index 0ca87be..d348395 100644
--- a/lib/lp/archivepublisher/artifactory.py
+++ b/lib/lp/archivepublisher/artifactory.py
@@ -7,6 +7,7 @@ __all__ = [
"ArtifactoryPool",
]
+from collections import defaultdict
import logging
import os
from pathlib import (
@@ -78,6 +79,15 @@ class ArtifactoryPoolEntry:
else:
raise AssertionError("Unsupported file: %r" % pub_file)
+ # Property names outside the "launchpad." namespace that we expect to
+ # overwrite. Any existing property names other than these will be left
+ # alone.
+ owned_properties = frozenset({
+ "deb.architecture",
+ "deb.component",
+ "deb.distribution",
+ })
+
def calculateProperties(self, release_id, publications):
"""Return a dict of Artifactory properties to set for this file.
@@ -117,8 +127,8 @@ class ArtifactoryPoolEntry:
used to generate more specific repositories.
"""
properties = {}
- properties["launchpad.release-id"] = release_id
- properties["launchpad.source-name"] = self.source
+ properties["launchpad.release-id"] = [release_id]
+ properties["launchpad.source-name"] = [self.source]
if publications:
archives = {publication.archive for publication in publications}
if len(archives) > 1:
@@ -139,16 +149,11 @@ class ArtifactoryPoolEntry:
if architectures:
properties["deb.architecture"] = architectures
else:
- properties["launchpad.channel"] = [
+ properties["launchpad.channel"] = sorted({
"%s:%s" % (
pub.distroseries.getSuite(pub.pocket),
pub.channel_string)
- for pub in sorted(
- publications,
- key=lambda p: (
- p.distroseries.version, p.pocket,
- p.channel_string))
- ]
+ for pub in publications})
return properties
def addFile(self, pub_file: IPackageReleaseFile):
@@ -184,13 +189,25 @@ class ArtifactoryPoolEntry:
if old_properties is None:
old_properties = targetpath.properties
release_id = old_properties.get("launchpad.release-id")
- if release_id is None:
+ if not release_id:
raise AssertionError(
"Cannot update properties: launchpad.release-id is not in %s" %
old_properties)
- properties = self.calculateProperties(release_id, publications)
- if old_properties != properties:
- targetpath.properties = properties
+ properties = self.calculateProperties(release_id[0], publications)
+ new_properties = {
+ key: value for key, value in old_properties.items()
+ if not key.startswith("launchpad.") and
+ key not in self.owned_properties}
+ new_properties.update(properties)
+ if old_properties != new_properties:
+ # We could use the ArtifactoryPath.properties setter, but that
+ # will fetch the old properties again when we already have them
+ # in hand; this approach saves an HTTP request.
+ properties_to_remove = set(old_properties) - set(new_properties)
+ if properties_to_remove:
+ targetpath.del_properties(
+ properties_to_remove, recursive=False)
+ targetpath.set_properties(new_properties, recursive=False)
def removeFile(self) -> int:
targetpath = self.pathFor()
@@ -210,11 +227,6 @@ class ArtifactoryPool:
def __init__(self, rootpath, logger: logging.Logger) -> None:
if not isinstance(rootpath, ArtifactoryPath):
rootpath = ArtifactoryPath(rootpath)
- write_creds = config.artifactory.write_credentials
- if write_creds is not None:
- # The X-JFrog-Art-Api header only needs the API key, not the
- # username.
- rootpath.auth = XJFrogArtApiAuth(write_creds.split(":", 1)[1])
rootpath.session = self._makeSession()
rootpath.timeout = config.launchpad.urlfetch_timeout
self.rootpath = rootpath
@@ -233,10 +245,15 @@ class ArtifactoryPool:
if config.launchpad.http_proxy:
session.proxies = {
"http": config.launchpad.http_proxy,
- "https": config.launchpad.https_proxy,
+ "https": config.launchpad.http_proxy,
}
if config.launchpad.ca_certificates_path is not None:
session.verify = config.launchpad.ca_certificates_path
+ write_creds = config.artifactory.write_credentials
+ if write_creds is not None:
+ # The X-JFrog-Art-Api header only needs the API key, not the
+ # username.
+ session.auth = XJFrogArtApiAuth(write_creds.split(":", 1)[1])
return session
def _getEntry(self, sourcename, file) -> ArtifactoryPoolEntry:
@@ -359,8 +376,15 @@ class ArtifactoryPool:
# non-admin users must include all of "name", "repo", and "path"
# in the include directive.
["repo", "path", "name", "property"])
- return {
- PurePath(artifact["path"], artifact["name"]): {
- prop["key"]: prop["value"] for prop in artifact["properties"]
- }
- for artifact in artifacts}
+ artifacts_by_path = {}
+ for artifact in artifacts:
+ path = PurePath(artifact["path"], artifact["name"])
+ properties = defaultdict(set)
+ for prop in artifact["properties"]:
+ properties[prop["key"]].add(prop["value"])
+ # AQL returns each value of multi-value properties separately
+ # and in an undefined order. Always sort them to ensure that we
+ # can compare properties reliably.
+ artifacts_by_path[path] = {
+ key: sorted(values) for key, values in properties.items()}
+ return artifacts_by_path
diff --git a/lib/lp/archivepublisher/tests/artifactory_fixture.py b/lib/lp/archivepublisher/tests/artifactory_fixture.py
index 1503afa..ba3099f 100644
--- a/lib/lp/archivepublisher/tests/artifactory_fixture.py
+++ b/lib/lp/archivepublisher/tests/artifactory_fixture.py
@@ -104,6 +104,17 @@ class FakeArtifactoryFixture(Fixture):
else:
return 404, {}, "Unable to find item"
+ def _decode_properties(self, encoded):
+ """Decode properties that were encoded as part of a request."""
+ properties = {}
+ for param in encoded.split(";"):
+ key, value = param.split("=", 1)
+ # XXX cjwatson 2022-04-19: Check whether this unquoting approach
+ # actually matches the artifactory module and Artifactory
+ # itself.
+ properties[unquote(key)] = [unquote(v) for v in value.split(",")]
+ return properties
+
def _handle_upload(self, request):
"""Handle a request to upload a directory or file."""
parsed_url = urlparse(request.url[len(self.repo_url):])
@@ -111,10 +122,7 @@ class FakeArtifactoryFixture(Fixture):
if path.endswith("/"):
self.add_dir(path.rstrip("/"))
elif path.rsplit("/", 1)[0] in self._fs:
- properties = {}
- for param in parsed_url.params.split(";"):
- key, value = param.split("=", 1)
- properties[unquote(key)] = unquote(value)
+ properties = self._decode_properties(parsed_url.params)
self.add_file(
path, request.body,
int(request.headers["Content-Length"]), properties)
@@ -126,9 +134,8 @@ class FakeArtifactoryFixture(Fixture):
path = parsed_url.path
if path in self._fs:
query = parse_qs(parsed_url.query)
- for param in query["properties"][0].split(";"):
- key, value = param.split("=", 1)
- self._fs[path]["properties"][unquote(key)] = unquote(value)
+ properties = self._decode_properties(query["properties"][0])
+ self._fs[path]["properties"].update(properties)
return 204, {}, ""
else:
return 404, {}, "Unable to find item"
@@ -153,9 +160,10 @@ class FakeArtifactoryFixture(Fixture):
"path": path_obj.parent.as_posix()[1:],
"name": path_obj.name,
"properties": [
- {"key": key, "value": value}
+ {"key": key, "value": v}
for key, value in sorted(
- self._fs[path]["properties"].items())],
+ self._fs[path]["properties"].items())
+ for v in value],
}
def _matches_aql(self, item, criteria):
diff --git a/lib/lp/archivepublisher/tests/test_artifactory.py b/lib/lp/archivepublisher/tests/test_artifactory.py
index 52ec77b..31bfbb1 100644
--- a/lib/lp/archivepublisher/tests/test_artifactory.py
+++ b/lib/lp/archivepublisher/tests/test_artifactory.py
@@ -3,7 +3,6 @@
"""Artifactory pool tests."""
-from operator import attrgetter
from pathlib import PurePath
import transaction
@@ -89,8 +88,8 @@ class TestArtifactoryPool(TestCase):
self.assertTrue(foo.checkIsFile())
self.assertEqual(
{
- "launchpad.release-id": "binary:1",
- "launchpad.source-name": "foo",
+ "launchpad.release-id": ["binary:1"],
+ "launchpad.source-name": ["foo"],
},
foo.getProperties())
@@ -156,12 +155,12 @@ class TestArtifactoryPool(TestCase):
self.assertEqual(
{
PurePath("pool/f/foo/foo-1.0.deb"): {
- "launchpad.release-id": "binary:1",
- "launchpad.source-name": "foo",
+ "launchpad.release-id": ["binary:1"],
+ "launchpad.source-name": ["foo"],
},
PurePath("pool/f/foo/foo-1.1.deb"): {
- "launchpad.release-id": "binary:2",
- "launchpad.source-name": "foo",
+ "launchpad.release-id": ["binary:2"],
+ "launchpad.source-name": ["foo"],
},
},
self.pool.getAllArtifacts(
@@ -169,8 +168,8 @@ class TestArtifactoryPool(TestCase):
self.assertEqual(
{
PurePath("pool/b/bar/bar-1.0.whl"): {
- "launchpad.release-id": "binary:3",
- "launchpad.source-name": "bar",
+ "launchpad.release-id": ["binary:3"],
+ "launchpad.source-name": ["bar"],
},
},
self.pool.getAllArtifacts(
@@ -215,17 +214,17 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
self.assertFalse(path.is_symlink())
self.assertEqual(
{
- "launchpad.release-id": "source:%d" % spr.id,
- "launchpad.source-name": "foo",
+ "launchpad.release-id": ["source:%d" % spr.id],
+ "launchpad.source-name": ["foo"],
},
path.properties)
self.pool.updateProperties(spr.name, sprf.libraryfile.filename, spphs)
self.assertEqual(
{
- "launchpad.release-id": "source:%d" % spr.id,
- "launchpad.source-name": "foo",
- "deb.distribution": ",".join(sorted(ds.name for ds in dses)),
- "deb.component": "main",
+ "launchpad.release-id": ["source:%d" % spr.id],
+ "launchpad.source-name": ["foo"],
+ "deb.distribution": list(sorted(ds.name for ds in dses)),
+ "deb.component": ["main"],
},
path.properties)
@@ -263,21 +262,19 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
self.assertFalse(path.is_symlink())
self.assertEqual(
{
- "launchpad.release-id": "binary:%d" % bpr.id,
- "launchpad.source-name": "foo",
+ "launchpad.release-id": ["binary:%d" % bpr.id],
+ "launchpad.source-name": ["foo"],
},
path.properties)
self.pool.updateProperties(
bpr.sourcepackagename, bpf.libraryfile.filename, bpphs)
self.assertEqual(
{
- "launchpad.release-id": "binary:%d" % bpr.id,
- "launchpad.source-name": "foo",
- "deb.distribution": ",".join(sorted(
- ds.name
- for ds in sorted(dses, key=attrgetter("version")))),
- "deb.component": "main",
- "deb.architecture": processor.name,
+ "launchpad.release-id": ["binary:%d" % bpr.id],
+ "launchpad.source-name": ["foo"],
+ "deb.distribution": list(sorted(ds.name for ds in dses)),
+ "deb.component": ["main"],
+ "deb.architecture": [processor.name],
},
path.properties)
@@ -309,19 +306,64 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
self.assertFalse(path.is_symlink())
self.assertEqual(
{
- "launchpad.release-id": "binary:%d" % bpr.id,
- "launchpad.source-name": "foo",
+ "launchpad.release-id": ["binary:%d" % bpr.id],
+ "launchpad.source-name": ["foo"],
},
path.properties)
self.pool.updateProperties(
bpr.sourcepackagename, bpf.libraryfile.filename, bpphs)
self.assertEqual(
{
- "launchpad.release-id": "binary:%d" % bpr.id,
- "launchpad.source-name": "foo",
- "deb.distribution": ds.name,
- "deb.component": "main",
- "deb.architecture": ",".join(sorted(
+ "launchpad.release-id": ["binary:%d" % bpr.id],
+ "launchpad.source-name": ["foo"],
+ "deb.distribution": [ds.name],
+ "deb.component": ["main"],
+ "deb.architecture": list(sorted(
das.architecturetag for das in dases)),
},
path.properties)
+
+ def test_updateProperties_preserves_externally_set_properties(self):
+ # Artifactory sets some properties by itself as part of scanning
+ # packages. We leave those untouched.
+ archive = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+ ds = self.factory.makeDistroSeries(distribution=archive.distribution)
+ das = self.factory.makeDistroArchSeries(distroseries=ds)
+ bpb = self.factory.makeBinaryPackageBuild(
+ archive=archive, distroarchseries=das,
+ pocket=PackagePublishingPocket.RELEASE, sourcepackagename="foo")
+ bpr = self.factory.makeBinaryPackageRelease(
+ binarypackagename="foo", build=bpb, component="main",
+ architecturespecific=False)
+ bpf = self.factory.makeBinaryPackageFile(
+ binarypackagerelease=bpr,
+ library_file=self.factory.makeLibraryFileAlias(
+ filename="foo_1.0_all.deb"),
+ filetype=BinaryPackageFileType.DEB)
+ bpphs = getUtility(IPublishingSet).publishBinaries(
+ archive, ds, PackagePublishingPocket.RELEASE,
+ {bpr: (bpr.component, bpr.section, bpr.priority, None)})
+ transaction.commit()
+ self.pool.addFile(
+ None, bpr.sourcepackagename, bpf.libraryfile.filename, bpf)
+ path = self.pool.rootpath / "f" / "foo" / "foo_1.0_all.deb"
+ path.set_properties({"deb.version": ["1.0"]}, recursive=False)
+ self.assertEqual(
+ {
+ "launchpad.release-id": ["binary:%d" % bpr.id],
+ "launchpad.source-name": ["foo"],
+ "deb.version": ["1.0"],
+ },
+ path.properties)
+ self.pool.updateProperties(
+ bpr.sourcepackagename, bpf.libraryfile.filename, bpphs)
+ self.assertEqual(
+ {
+ "launchpad.release-id": ["binary:%d" % bpr.id],
+ "launchpad.source-name": ["foo"],
+ "deb.distribution": [ds.name],
+ "deb.component": ["main"],
+ "deb.architecture": [das.architecturetag],
+ "deb.version": ["1.0"],
+ },
+ path.properties)
