Commented on the while True loop. Do we have tests for this? Diff comments:
> diff --git a/lib/lp/archivepublisher/artifactory.py > b/lib/lp/archivepublisher/artifactory.py > index 08eb83a..413cef3 100644 > --- a/lib/lp/archivepublisher/artifactory.py > +++ b/lib/lp/archivepublisher/artifactory.py > @@ -636,31 +636,43 @@ class ArtifactoryPool: > # length): > # https://www.jfrog.com/confluence/display/JFROG/\ > # Artifactory+Query+Language > - artifacts = self.rootpath.aql( > - "items.find", > - { > - "repo": repository_name, > - "$or": [ > - {"name": {"$match": pattern}} > - for pattern in > self.getArtifactPatterns(repository_format) > - ], > - }, > - ".include", > - # We don't use "repo", but the AQL documentation says that > - # non-admin users must include all of "name", "repo", and "path" > - # in the include directive. > - ["repo", "path", "name", "property"], > - ) > + > + offset = 0 > + limit = 100000 > 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.get("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() > - } > + > + while True: Don't really like a `while True`, imagine the API is broken and just keeps returning the same thing. Can we put a certain number of loops here and if it gets to the max we oops. > + artifacts = self.rootpath.aql( > + "items.find", > + { > + "repo": repository_name, > + "$or": [ > + {"name": {"$match": pattern}} > + for pattern in self.getArtifactPatterns( > + repository_format > + ) > + ], > + }, > + ".include", > + # We don't use "repo", but the AQL documentation says that > + # non-admin users must include all of "name", "repo", > + # and "path" in the include directive. > + ["repo", "path", "name", "property"], > + ".offset(%s)" % offset, > + ".limit(%s)" % limit, > + ) > + if len(artifacts) == 0: > + break > + for artifact in artifacts: > + path = PurePath(artifact["path"], artifact["name"]) > + properties = defaultdict(set) > + for prop in artifact["properties"]: > + properties[prop["key"]].add(prop.get("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() > + } > + offset += limit > return artifacts_by_path -- https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/478395 Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:artifactory-properties-missing into launchpad:master. _______________________________________________ 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
