Re: [Launchpad-reviewers] [Merge] ~barryprice/launchpad-mojo-specs/+git/private:vbuilder into ~launchpad/launchpad-mojo-specs/+git/private:vbuilder

[William Grant]

Review: Approve


-- 
https://code.launchpad.net/~barryprice/launchpad-mojo-specs/+git/private/+merge/463363
Your team Launchpad code reviewers is subscribed to branch 
~launchpad/launchpad-mojo-specs/+git/private:vbuilder.


___
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

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:buildinfo-exposure into launchpad:master

[William Grant]

William Grant has proposed merging ~wgrant/launchpad:buildinfo-exposure into 
launchpad:master.

Commit message:
Expose .buildinfo files in the API and web UI

We've been capturing these for BinaryPackageBuilds for years, and
storing them in the DB and librarian, but they haven't been available to
users.

LP: #1686242

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1686242 in Launchpad itself: ".changes files reference .buildinfo files 
that aren't exposed"
  https://bugs.launchpad.net/launchpad/+bug/1686242

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/459249
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:buildinfo-exposure into launchpad:master.
diff --git a/lib/lp/soyuz/doc/build-files.rst b/lib/lp/soyuz/doc/build-files.rst
index ddca43d..2ba7f52 100644
--- a/lib/lp/soyuz/doc/build-files.rst
+++ b/lib/lp/soyuz/doc/build-files.rst
@@ -28,6 +28,7 @@ following file type in its context.
  * Binary changesfile: '.changes';
  * Build logs: '.txt.gz';
  * Build upload logs: '_log.txt';
+ * .buildinfo file: '.buildinfo';
  * Built files: '*deb';
 
 >>> print(build.title)
@@ -70,6 +71,14 @@ Adding and retrieving a upload_log.
 >>> build.upload_log == build.getFileByName(upload_log_name)
 True
 
+Adding and retrieving a .buildinfo.
+
+>>> build.addBuildInfo(
+... factory.makeLibraryFileAlias(filename="foo.buildinfo")
+... )
+>>> build.buildinfo == build.getFileByName("foo.buildinfo")
+True
+
 Retrieve a built file:
 
 >>> deb == build.getFileByName("test_1.0_all.deb")
diff --git a/lib/lp/soyuz/interfaces/binarypackagebuild.py b/lib/lp/soyuz/interfaces/binarypackagebuild.py
index 18bc50f..e72b450 100644
--- a/lib/lp/soyuz/interfaces/binarypackagebuild.py
+++ b/lib/lp/soyuz/interfaces/binarypackagebuild.py
@@ -139,8 +139,9 @@ class IBinaryPackageBuildView(IPackageBuildView):
 
 changesfile_url = exported(
 TextLine(
-title=_("Changes File URL"),
+title=_("Changes file URL"),
 required=False,
+readonly=True,
 description=_(
 "The URL for the changes file for this build. "
 "Will be None if the build was imported by Gina."
@@ -153,6 +154,15 @@ class IBinaryPackageBuildView(IPackageBuildView):
 "this build, if any."
 )
 
+buildinfo_url = exported(
+TextLine(
+title=_("buildinfo file URL"),
+required=False,
+readonly=True,
+description=_("The URL for the .buildinfo file for this build."),
+)
+)
+
 package_upload = Attribute(
 "The `PackageUpload` record corresponding to the original upload "
 "of the binaries resulted from this build. It's 'None' if it is "
diff --git a/lib/lp/soyuz/model/binarypackagebuild.py b/lib/lp/soyuz/model/binarypackagebuild.py
index a801a6a..4ac614c 100644
--- a/lib/lp/soyuz/model/binarypackagebuild.py
+++ b/lib/lp/soyuz/model/binarypackagebuild.py
@@ -408,6 +408,14 @@ class BinaryPackageBuild(PackageBuildMixin, StormBase):
 return ProxiedLibraryFileAlias(self.upload_log, self).http_url
 
 @property
+def buildinfo_url(self):
+"""See `IBinaryPackageBuild`."""
+buildinfo = self.buildinfo
+if buildinfo is None:
+return None
+return ProxiedLibraryFileAlias(buildinfo, self).http_url
+
+@property
 def distributionsourcepackagerelease(self):
 """See `IBuild`."""
 from lp.soyuz.model.distributionsourcepackagerelease import (
@@ -832,6 +840,8 @@ class BinaryPackageBuild(PackageBuildMixin, StormBase):
 """See `IBuild`."""
 if filename.endswith(".changes"):
 file_object = self.upload_changesfile
+elif filename.endswith(".buildinfo"):
+file_object = self.buildinfo
 elif filename.endswith(".txt.gz"):
 file_object = self.log
 elif is_upload_log(filename):
diff --git a/lib/lp/soyuz/stories/soyuz/xx-build-record.rst b/lib/lp/soyuz/stories/soyuz/xx-build-record.rst
index 9da0cb4..593e1c7 100644
--- a/lib/lp/soyuz/stories/soyuz/xx-build-record.rst
+++ b/lib/lp/soyuz/stories/soyuz/xx-build-record.rst
@@ -310,6 +310,7 @@ appropriate 'Build status' section the user will see 2 new sections,
 >>> build.buildqueue_record.destroySelf()
 >>> build.updateStatus(BuildStatus.FULLYBUILT, builder=bob_builder)
 >>> build.setLog(stp.addMockFile("fake-buildlog"))
+>>> build.addBuildInfo(stp.addMockFile("testing_1.0_all.buil

Re: [Launchpad-reviewers] [Merge] ~ines-almeida/launchpad:db-add-social-accounts-table into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/457880
Your team Launchpad code reviewers is requested to review the proposed merge of 
~ines-almeida/launchpad:db-add-social-accounts-table into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~ines-almeida/launchpad:db-add-social-accounts-table into launchpad:db-devel

[William Grant]

Launchpad PostgreSQL table names are lowercase and singular: it should be 
"socialaccount".

Should we have an index on platform as well, to make debugging and maintenance 
operations easier?

Could this replace the existing ircid, jabberid, wikiname tables?
-- 
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/457221
Your team Launchpad code reviewers is requested to review the proposed merge of 
~ines-almeida/launchpad:db-add-social-accounts-table into launchpad:db-devel.


___
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

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:buildd-manager-nicer-retries into launchpad:master

[William Grant]

William Grant has proposed merging 
~wgrant/launchpad:buildd-manager-nicer-retries into launchpad:master with 
~wgrant/launchpad:buildd-manager-failure-metrics as a prerequisite.

Commit message:
Cope more gracefully with intermittent builder glitches

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/454694

buildd-manager would previously immediately count any single scan
failure against the builder and job. This meant that three glitches --
say, network timeouts -- over the course of job would result in the
build being requeued. A builder's failure count is reset on successful
dispatch, but a job's deliberately isn't since we want to fail builds
that are repeatedly killing builders. This meant that a single network
glitch in the second attempt at a build would cause it to be failed.

This added layer of failure counting substantially reduces the
likelihood of those two scenarios, by requiring five consecutive
unsuccessful scans before a single failure is counted against a builder
or job. This means that brief network interruptions, or indeed temporary
insanity on buildd-manager's part, should no longer cause builds to be
requeued or failed at all.

The only significant downside of this change is that recovery from
legitimate failures will now take a few minutes longer. But that's much
less of a concern with the very large build farm we have nowadays.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:buildd-manager-nicer-retries into launchpad:master.
diff --git a/lib/lp/buildmaster/manager.py b/lib/lp/buildmaster/manager.py
index 2bc0b81..372b637 100644
--- a/lib/lp/buildmaster/manager.py
+++ b/lib/lp/buildmaster/manager.py
@@ -54,6 +54,10 @@ from lp.services.statsd.interfaces.statsd_client import IStatsdClient
 BUILDD_MANAGER_LOG_NAME = "worker-scanner"
 
 
+# The number of times the scan of a builder can fail before we start
+# attributing it to the builder and job.
+SCAN_FAILURE_THRESHOLD = 5
+
 # The number of times a builder can consecutively fail before we
 # reset its current job.
 JOB_RESET_THRESHOLD = 3
@@ -515,6 +519,12 @@ class WorkerScanner:
 
 self.can_retry = True
 
+# The build and job failure counts are persisted, but we only really
+# care about the consecutive scan failure count over the space of a
+# couple of minutes, so it's okay if it resets on buildd-manager
+# restart.
+self.scan_failure_count = 0
+
 # We cache the build cookie, keyed on the BuildQueue, to avoid
 # hitting the DB on every scan.
 self._cached_build_cookie = None
@@ -553,6 +563,12 @@ class WorkerScanner:
 
 try:
 yield self.scan()
+
+# We got through a scan without error, so reset the consecutive
+# failure count. We don't reset the persistent builder or job
+# failure counts, since the build might consistently break the
+# builder later in the build.
+self.scan_failure_count = 0
 except Exception as e:
 self._scanFailed(self.can_retry, e)
 
@@ -591,12 +607,35 @@ class WorkerScanner:
 "Scanning %s failed" % self.builder_name, exc_info=exc
 )
 
+# Certain errors can't possibly be a glitch, and they can insta-fail
+# even if the scan phase would normally allow a retry.
+if isinstance(exc, (BuildDaemonIsolationError, CannotResumeHost)):
+retry = False
+
 # Decide if we need to terminate the job or reset/fail the builder.
 vitals = self.builder_factory.getVitals(self.builder_name)
 builder = self.builder_factory[self.builder_name]
 try:
 labels = get_statsd_labels(builder, builder.current_build)
 self.statsd_client.incr("builders.failure", labels=labels)
+self.scan_failure_count += 1
+
+# To avoid counting network glitches or similar against innocent
+# builds and jobs, we allow a scan to fail a few times in a row
+# without consequence, and just retry. If we exceed the threshold,
+# we then persistently record a single failure against the build
+# and job.
+if retry and self.scan_failure_count < SCAN_FAILURE_THRESHOLD:
+self.statsd_client.incr(
+"builders.failure.scan_retried",
+labels={
+"failures": str(self.scan_failure_count),
+**labels,
+},
+)
+return
+
+self.scan_failure_count = 0
 
 builder.gotFailure()
 if builder.current_build is not None:
@@ -610,6 +649,7 @@ class WorkerScanner:
 "Miserable failure when tr

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:buildd-manager-failure-metrics into launchpad:master

[William Grant]

The proposal to merge ~wgrant/launchpad:buildd-manager-failure-metrics into 
launchpad:master has been updated.

Description changed to:

They all now start with "builders.failure", labels are more consistent,
and builder failure/reset are counted.

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/454693
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:buildd-manager-failure-metrics 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

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:buildd-manager-failure-metrics into launchpad:master

[William Grant]

The proposal to merge ~wgrant/launchpad:buildd-manager-failure-metrics into 
launchpad:master has been updated.

Commit message changed to:

Tweak buildd-manager failure handling metrics

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/454693
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:buildd-manager-failure-metrics 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

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:buildd-manager-failure-metrics into launchpad:master

[William Grant]

William Grant has proposed merging 
~wgrant/launchpad:buildd-manager-failure-metrics into launchpad:master with 
~wgrant/launchpad:buildd-manager-failure-refactor as a prerequisite.

Commit message:
Tweak buildd-manager failure handling metrics

They all now start with "builders.failure", labels are more consistent,
and builder failure/reset are counted.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/454693
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:buildd-manager-failure-metrics into launchpad:master.
diff --git a/lib/lp/buildmaster/manager.py b/lib/lp/buildmaster/manager.py
index 0c6d335..2bc0b81 100644
--- a/lib/lp/buildmaster/manager.py
+++ b/lib/lp/buildmaster/manager.py
@@ -312,6 +312,23 @@ class PrefetchedBuilderFactory(BaseBuilderFactory):
 return self.candidates.pop(vitals)
 
 
+def get_statsd_labels(builder, build):
+labels = {
+"builder_name": builder.name,
+"region": builder.region,
+"virtualized": str(builder.virtualized),
+}
+if build is not None:
+labels.update(
+{
+"build": True,
+"arch": build.processor.name,
+"job_type": build.job_type.name,
+}
+)
+return labels
+
+
 def judge_failure(builder_count, job_count, exc, retry=True):
 """Judge how to recover from a scan failure.
 
@@ -394,22 +411,15 @@ def recover_failure(logger, vitals, builder, retry, exception):
 job_action,
 )
 
-if job is not None and job_action is not None:
-statsd_client = getUtility(IStatsdClient)
-labels = {
-"build": True,
-"arch": job.specific_build.processor.name,
-"region": builder.region,
-"builder_name": builder.name,
-"virtualized": str(builder.virtualized),
-"job_type": job.specific_build.job_type.name,
-}
+statsd_client = getUtility(IStatsdClient)
+labels = get_statsd_labels(builder, job.specific_build if job else None)
 
+if job is not None and job_action is not None:
 if cancelling:
 # We've previously been asked to cancel the job, so just set
 # it to cancelled rather than retrying or failing.
 logger.info("Cancelling job %s.", job.build_cookie)
-statsd_client.incr("builders.job_cancelled", labels=labels)
+statsd_client.incr("builders.failure.job_cancelled", labels=labels)
 job.markAsCancelled()
 elif job_action == False:
 # Fail and dequeue the job.
@@ -430,12 +440,12 @@ def recover_failure(logger, vitals, builder, retry, exception):
 job.specific_build.updateStatus(
 BuildStatus.FAILEDTOBUILD, force_invalid_transition=True
 )
-statsd_client.incr("builders.job_failed", labels=labels)
+statsd_client.incr("builders.failure.job_failed", labels=labels)
 job.destroySelf()
 elif job_action == True:
 # Reset the job so it will be retried elsewhere.
 logger.info("Requeueing job %s.", job.build_cookie)
-statsd_client.incr("builders.job_reset", labels=labels)
+statsd_client.incr("builders.failure.job_reset", labels=labels)
 job.reset()
 
 if job_action == False:
@@ -447,12 +457,14 @@ def recover_failure(logger, vitals, builder, retry, exception):
 # We've already tried resetting it enough times, so we have
 # little choice but to give up.
 logger.info("Failing builder %s.", builder.name)
+statsd_client.incr("builders.failure.builder_failed", labels=labels)
 builder.failBuilder(str(exception))
 elif builder_action == True:
 # Dirty the builder to attempt recovery. In the virtual case,
 # the dirty idleness will cause a reset, giving us a good chance
 # of recovery.
 logger.info("Dirtying builder %s to attempt recovery.", builder.name)
+statsd_client.incr("builders.failure.builder_reset", labels=labels)
 builder.setCleanStatus(BuilderCleanStatus.DIRTY)
 
 
@@ -583,20 +595,13 @@ class WorkerScanner:
 vitals = self.builder_factory.getVitals(self.builder_name)
 builder = self.builder_factory[self.builder_name]
 try:
+labels = get_statsd_labels(builder, builder.current_build)
+self.statsd_client.incr("builders.failure", labels=labels)
+
 builder.gotFailure()
-labe

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:buildd-manager-failure-refactor into launchpad:master

[William Grant]

William Grant has proposed merging 
~wgrant/launchpad:buildd-manager-failure-refactor into launchpad:master.

Commit message:
Refactor buildd-manager job dispatch error handling

manager.py is now entirely inlineCallbacks.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/454692
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:buildd-manager-failure-refactor into launchpad:master.
diff --git a/lib/lp/buildmaster/manager.py b/lib/lp/buildmaster/manager.py
index 7f2b870..0c6d335 100644
--- a/lib/lp/buildmaster/manager.py
+++ b/lib/lp/buildmaster/manager.py
@@ -11,7 +11,6 @@ __all__ = [
 ]
 
 import datetime
-import functools
 import logging
 import os.path
 import shutil
@@ -502,6 +501,8 @@ class WorkerScanner:
 self.date_cancel = None
 self.date_scanned = None
 
+self.can_retry = True
+
 # We cache the build cookie, keyed on the BuildQueue, to avoid
 # hitting the DB on every scan.
 self._cached_build_cookie = None
@@ -520,6 +521,7 @@ class WorkerScanner:
 """Terminate the LoopingCall."""
 self.loop.stop()
 
+@defer.inlineCallbacks
 def singleCycle(self):
 # Inhibit scanning if the BuilderFactory hasn't updated since
 # the last run. This doesn't matter for the base BuilderFactory,
@@ -533,22 +535,19 @@ class WorkerScanner:
 self.logger.debug(
 "Skipping builder %s (cache out of date)" % self.builder_name
 )
-return defer.succeed(None)
+return
 
 self.logger.debug("Scanning builder %s" % self.builder_name)
-# Errors should normally be able to be retried a few times. Bits
-# of scan() which don't want retries will call _scanFailed
-# directly.
-d = self.scan()
-d.addErrback(functools.partial(self._scanFailed, True))
-d.addBoth(self._updateDateScanned)
-return d
 
-def _updateDateScanned(self, ignored):
+try:
+yield self.scan()
+except Exception as e:
+self._scanFailed(self.can_retry, e)
+
 self.logger.debug("Scan finished for builder %s" % self.builder_name)
 self.date_scanned = datetime.datetime.utcnow()
 
-def _scanFailed(self, retry, failure):
+def _scanFailed(self, retry, exc):
 """Deal with failures encountered during the scan cycle.
 
 1. Print the error in the log
@@ -562,26 +561,22 @@ class WorkerScanner:
 
 # If we don't recognise the exception include a stack trace with
 # the error.
-error_message = failure.getErrorMessage()
-if failure.check(
-BuildWorkerFailure,
-CannotBuild,
-CannotResumeHost,
-BuildDaemonError,
-CannotFetchFile,
+if isinstance(
+exc,
+(
+BuildWorkerFailure,
+CannotBuild,
+CannotResumeHost,
+BuildDaemonError,
+CannotFetchFile,
+),
 ):
 self.logger.info(
-"Scanning %s failed with: %s"
-% (self.builder_name, error_message)
+"Scanning %s failed with: %r" % (self.builder_name, exc)
 )
 else:
 self.logger.info(
-"Scanning %s failed with: %s\n%s"
-% (
-self.builder_name,
-failure.getErrorMessage(),
-failure.getTraceback(),
-)
+"Scanning %s failed" % self.builder_name, exc_info=exc
 )
 
 # Decide if we need to terminate the job or reset/fail the builder.
@@ -602,7 +597,7 @@ class WorkerScanner:
 else:
 labels["build"] = False
 self.statsd_client.incr("builders.judged_failed", labels=labels)
-recover_failure(self.logger, vitals, builder, retry, failure.value)
+recover_failure(self.logger, vitals, builder, retry, exc)
 transaction.commit()
 except Exception:
 # Catastrophic code failure! Not much we can do.
@@ -687,6 +682,7 @@ class WorkerScanner:
 vitals = self.builder_factory.getVitals(self.builder_name)
 interactor = self.interactor_factory()
 worker = self.worker_factory(vitals)
+self.can_retry = True
 
 if vitals.build_queue is not None:
 if vitals.clean_status != BuilderCleanStatus.DIRTY:
@@ -759,15 +755,14 @@ class WorkerScanner:
 "%s is in manual mode, not dispatching.", vitals.name
 )
 return
-# Try

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:archive-file-history-refactor into launchpad:master

[William Grant]

The proposal to merge ~wgrant/launchpad:archive-file-history-refactor into 
launchpad:master has been updated.

Status: Work in progress => Rejected

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/390811
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:archive-file-history-refactor 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

Re: [Launchpad-reviewers] [Merge] ~xnox/launchpad:drop-i18n-index into launchpad:master

[William Grant]

Tools like debmirror used to use this. Have we confirmed that other common 
tools handling apt repositories no longer do?
-- 
https://code.launchpad.net/~xnox/launchpad/+git/launchpad/+merge/453586
Your team Launchpad code reviewers is requested to review the proposed merge of 
~xnox/launchpad:drop-i18n-index 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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad-mojo-specs/+git/private:vbuilder-production-riscv64 into ~launchpad/launchpad-mojo-specs/+git/private:vbuilder

[William Grant]

Review: Approve


-- 
https://code.launchpad.net/~cjwatson/launchpad-mojo-specs/+git/private/+merge/453216
Your team Launchpad code reviewers is subscribed to branch 
~launchpad/launchpad-mojo-specs/+git/private:vbuilder.


___
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

Re: [Launchpad-reviewers] [Merge] ~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into turnip:master

[William Grant]

This looks good to me now, thanks!
-- 
https://code.launchpad.net/~bowenfan/turnip/+git/turnip/+merge/452540
Your team Launchpad code reviewers is requested to review the proposed merge of 
~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into 
turnip: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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad-mojo-specs/+git/private:vbuilder-qastaging-riscv64 into ~launchpad/launchpad-mojo-specs/+git/private:vbuilder

[William Grant]

Review: Approve


-- 
https://code.launchpad.net/~cjwatson/launchpad-mojo-specs/+git/private/+merge/453131
Your team Launchpad code reviewers is subscribed to branch 
~launchpad/launchpad-mojo-specs/+git/private:vbuilder.


___
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

Re: [Launchpad-reviewers] [Merge] ~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into turnip:master

[William Grant]

Diff comments:

> diff --git a/turnip/api/views.py b/turnip/api/views.py
> index 5014634..4d64b62 100644
> --- a/turnip/api/views.py
> +++ b/turnip/api/views.py
> @@ -265,6 +265,61 @@ class RefAPI(BaseAPI):
>  return exc.HTTPNotFound()  # 404
>  
>  @validate_path
> +def collection_post(self, repo_store, repo_name):
> +"""Create a git reference against a commit."""
> +json_data = extract_cstruct(self.request)["body"]
> +commit_sha1 = json_data.get("commit_sha1")
> +ref = json_data.get("ref")
> +force = json_data.get("force", False)
> +
> +if not commit_sha1:
> +self.request.errors.add(
> +"body", "commit_sha1", "commit_sha1 for ref target is 
> missing"
> +)
> +return
> +if not ref:
> +self.request.errors.add("body", "ref", "must specify ref name")
> +return
> +# Do not accept truthy values (e.g. {"force": "no"}) to avoid 
> confusion
> +if force and not isinstance(force, bool):
> +self.request.errors.add(
> +"body", "force", "'force' must be a json boolean"
> +)
> +return
> +
> +ref = "refs/" + ref

Heh, that's fun, I expected libgit2 to stop that. OK, let's for now validate 
that they start with refs/.

> +try:
> +store.create_reference(
> +repo_store, repo_name, ref, commit_sha1, force
> +)
> +except AlreadyExistsError:
> +self.request.errors.add(
> +"body",
> +"ref",
> +"ref already exists",
> +)
> +self.request.errors.status = 409
> +return
> +except InvalidSpecError:
> +self.request.errors.add(
> +"body",
> +"ref",
> +"ensure ref name is valid per git-check-ref-format",
> +)
> +return
> +except GitError:
> +# This diverges from the generic Resource Not Found for get_ref,
> +# but as we are dealing with multiple object types here it would
> +# be helpful to raise a more specific error.
> +self.request.errors.add(
> +"body", "commit_sha1", "no commit found for commit_sha1 
> given"
> +)
> +self.request.errors.status = 404
> +return
> +else:
> +return Response(status=201)
> +
> +@validate_path
>  def get(self, repo_store, repo_name):
>  ref = "refs/" + self.request.matchdict["ref"]
>  try:


-- 
https://code.launchpad.net/~bowenfan/turnip/+git/turnip/+merge/452540
Your team Launchpad code reviewers is requested to review the proposed merge of 
~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into 
turnip: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

Re: [Launchpad-reviewers] [Merge] ~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into turnip:master

[William Grant]

This should also almost certainly be a bulk API. It's fine for it to not be 
totally transactional and perform partial writes on failure.
-- 
https://code.launchpad.net/~bowenfan/turnip/+git/turnip/+merge/452540
Your team Launchpad code reviewers is requested to review the proposed merge of 
~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into 
turnip: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

Re: [Launchpad-reviewers] [Merge] ~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into turnip:master

[William Grant]

Diff comments:

> diff --git a/turnip/api/store.py b/turnip/api/store.py
> index ae897c2..b4e8a33 100644
> --- a/turnip/api/store.py
> +++ b/turnip/api/store.py
> @@ -555,6 +563,23 @@ def get_ref(repo_store, repo_name, ref):
>  return ref_obj
>  
>  
> +def create_reference(repo_store, repo_name, ref, commit_sha1, force=False):
> +"""Create a git reference against a given commit sha1.
> +
> +:param ref: Name of the git ref to write. The client should send
> +'tags/' for tags and 'heads/'
> +for branches. The client is free to send other arbitrary paths.
> +:param commit_sha1: SHA1 hash of the ref commit target.
> +:param force: If True, force overwrite the original ref (if any)
> +to point to the new commit.
> +"""
> +with open_repo(repo_store, repo_name) as repo:
> +try:
> +repo.create_reference(ref, commit_sha1, force)
> +except (ObjectAlreadyExistsError, InvalidSpecError, GitError):
> +raise

Is it useful to catch three specific exceptions only to raise them again?

> +
> +
>  def get_common_ancestor_diff(
>  repo_store, repo_name, sha1_target, sha1_source, context_lines=3
>  ):
> diff --git a/turnip/api/views.py b/turnip/api/views.py
> index 5014634..4d64b62 100644
> --- a/turnip/api/views.py
> +++ b/turnip/api/views.py
> @@ -265,6 +265,61 @@ class RefAPI(BaseAPI):
>  return exc.HTTPNotFound()  # 404
>  
>  @validate_path
> +def collection_post(self, repo_store, repo_name):
> +"""Create a git reference against a commit."""
> +json_data = extract_cstruct(self.request)["body"]
> +commit_sha1 = json_data.get("commit_sha1")
> +ref = json_data.get("ref")
> +force = json_data.get("force", False)
> +
> +if not commit_sha1:
> +self.request.errors.add(
> +"body", "commit_sha1", "commit_sha1 for ref target is 
> missing"
> +)
> +return
> +if not ref:
> +self.request.errors.add("body", "ref", "must specify ref name")
> +return
> +# Do not accept truthy values (e.g. {"force": "no"}) to avoid 
> confusion
> +if force and not isinstance(force, bool):
> +self.request.errors.add(
> +"body", "force", "'force' must be a json boolean"
> +)
> +return
> +
> +ref = "refs/" + ref

Refs in Git are named `refs/SOME/PATH`; the `refs/` prefix isn't ever implicit. 
turnip's get_refs returns the `refs/` prefix, so this probably shouldn't 
prepend it.

> +try:
> +store.create_reference(
> +repo_store, repo_name, ref, commit_sha1, force
> +)
> +except AlreadyExistsError:
> +self.request.errors.add(
> +"body",
> +"ref",
> +"ref already exists",
> +)
> +self.request.errors.status = 409
> +return
> +except InvalidSpecError:
> +self.request.errors.add(
> +"body",
> +"ref",
> +"ensure ref name is valid per git-check-ref-format",

Should this directly state the error -- that the ref name is invalid?

> +)
> +return
> +except GitError:
> +# This diverges from the generic Resource Not Found for get_ref,
> +# but as we are dealing with multiple object types here it would
> +# be helpful to raise a more specific error.
> +self.request.errors.add(
> +"body", "commit_sha1", "no commit found for commit_sha1 
> given"
> +)
> +self.request.errors.status = 404
> +return
> +else:
> +return Response(status=201)

Could this be outside the else block?

> +
> +@validate_path
>  def get(self, repo_store, repo_name):
>  ref = "refs/" + self.request.matchdict["ref"]
>  try:


-- 
https://code.launchpad.net/~bowenfan/turnip/+git/turnip/+merge/452540
Your team Launchpad code reviewers is requested to review the proposed merge of 
~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into 
turnip: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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-name-blocklist into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/426843
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-name-blocklist into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] lp:~hloeung/canonical-mojo-specs/launchpad-fix-squid-warnings-reduce-cron-spam into lp:~canonical-launchpad-branches/canonical-mojo-specs/trunk

[William Grant]

Review: Approve


-- 
https://code.launchpad.net/~hloeung/canonical-mojo-specs/launchpad-fix-squid-warnings-reduce-cron-spam/+merge/428577
Your team Launchpad code reviewers is subscribed to branch 
lp:~canonical-launchpad-branches/canonical-mojo-specs/trunk.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-sync-comments into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/427020
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-sync-comments into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~lgp171188/launchpad:vulnerability-subscription-sql into launchpad:db-devel

[William Grant]

Review: Approve db



Diff comments:

> diff --git a/database/schema/patch-2211-02-0.sql 
> b/database/schema/patch-2211-02-0.sql
> new file mode 100644
> index 000..57f2be4
> --- /dev/null
> +++ b/database/schema/patch-2211-02-0.sql
> @@ -0,0 +1,115 @@
> +-- Copyright 2022 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +ALTER TABLE Vulnerability
> +ADD COLUMN access_policy integer,
> +ADD COLUMN access_grants integer[];
> +
> +CREATE TABLE VulnerabilitySubscription (
> +id serial PRIMARY KEY,
> +person integer REFERENCES Person NOT NULL,
> +vulnerability integer REFERENCES Vulnerability NOT NULL,
> +date_created timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT 
> TIME ZONE 'UTC') NOT NULL,
> +subscribed_by integer REFERENCES Person NOT NULL
> +);
> +
> +COMMENT ON TABLE VulnerabilitySubscription IS 'Person subscription for 
> Vulnerabilities.';
> +COMMENT ON COLUMN VulnerabilitySubscription.person IS 'The person 
> subscribing to the vulnerability.';
> +COMMENT ON COLUMN VulnerabilitySubscription.vulnerability IS 'The 
> vulnerability being subscribed to.';
> +COMMENT ON COLUMN VulnerabilitySubscription.date_created IS 'The date when 
> the subscription was created.';
> +COMMENT ON COLUMN VulnerabilitySubscription.subscribed_by IS 'The person who 
> created the subscription.';
> +
> +CREATE UNIQUE INDEX vulnerabilitysubscription__person__vulnerability__key
> +ON VulnerabilitySubscription (person, vulnerability);
> +
> +CREATE INDEX vulnerabilitysubscription__vulnerability__idx
> +ON VulnerabilitySubscription (vulnerability);
> +
> +CREATE INDEX vulnerabilitysubscription__subscribed_by__idx
> +ON VulnerabilitySubscription (subscribed_by);
> +
> +ALTER TABLE AccessArtifact
> +ADD COLUMN vulnerability integer REFERENCES Vulnerability;
> +
> +
> +ALTER TABLE AccessArtifact DROP CONSTRAINT has_artifact;
> +ALTER TABLE AccessArtifact
> +ADD CONSTRAINT has_artifact CHECK (
> +(null_count(ARRAY[bug, branch, gitrepository, snap, specification, 
> ocirecipe, vulnerability]) = 6)) NOT VALID;
> +
> +
> +CREATE OR REPLACE FUNCTION vulnerability_denorm_access(vulnerability_id 
> integer)
> +RETURNS void LANGUAGE plpgsql AS
> +$$
> +DECLARE
> +info_type integer;
> +BEGIN
> +SELECT
> +-- information type: 1 = public
> +COALESCE(Vulnerability.information_type, 1)

Vulnerability.information_type is NOT NULL, so I don't think this COALESCE is 
required.

> +INTO info_type
> +FROM Vulnerability where id = vulnerability_id;
> +
> +UPDATE Vulnerability
> +SET access_policy = policies[1], access_grants = grants
> +FROM
> +build_access_cache(
> +(SELECT id FROM accessartifact WHERE vulnerability = 
> vulnerability_id),
> +info_type)
> +AS (policies integer[], grants integer[])
> +WHERE id = vulnerability_id;
> +END;
> +$$;
> +
> +CREATE OR REPLACE FUNCTION accessartifact_denorm_to_artifacts(artifact_id 
> integer)
> +RETURNS void
> +LANGUAGE plpgsql
> +AS $$
> +DECLARE
> +artifact_row accessartifact%ROWTYPE;
> +BEGIN
> +SELECT * INTO artifact_row FROM accessartifact WHERE id = artifact_id;
> +IF artifact_row.bug IS NOT NULL THEN
> +PERFORM bug_flatten_access(artifact_row.bug);
> +END IF;
> +IF artifact_row.branch IS NOT NULL THEN
> +PERFORM branch_denorm_access(artifact_row.branch);
> +END IF;
> +IF artifact_row.gitrepository IS NOT NULL THEN
> +PERFORM gitrepository_denorm_access(artifact_row.gitrepository);
> +END IF;
> +IF artifact_row.snap IS NOT NULL THEN
> +PERFORM snap_denorm_access(artifact_row.snap);
> +END IF;
> +IF artifact_row.specification IS NOT NULL THEN
> +PERFORM specification_denorm_access(artifact_row.specification);
> +END IF;
> +IF artifact_row.ocirecipe IS NOT NULL THEN
> +PERFORM ocirecipe_denorm_access(artifact_row.ocirecipe);
> +END IF;
> +IF artifact_row.vulnerability IS NOT NULL THEN
> +PERFORM vulnerability_denorm_access(artifact_row.vulnerability);
> +END IF;
> +RETURN;
> +END;
> +$$;
> +
> +COMMENT ON FUNCTION accessartifact_denorm_to_artifacts(artifact_id integer) 
> IS
> +'Denormalize the policy access and artifact grants to bugs, branches, 
> git repositories, snaps, specifications, ocirecipes, and vulnerabilities.';
> +
> +-- A trigger to handle vulnerability.information_type changes.
> +CREATE OR REPLACE FUNCTION vulnerability_maintain_access_cache_trig() 
> RETURNS trigger
> +LANGUAGE plpgsql as $$
> +BEGIN
> +PERFORM vulnerability_denorm_access(NEW.id);
> +RETURN NULL;
> +END;
> +$$;
> +
> +CREATE TRIGGER vulnerability_maintain_access_cache
> +AFTER INSERT OR UPDATE OF information_type ON Vulnerability
> +FOR EACH ROW EXECUTE 

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-baseline-2211 into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/426916
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-baseline-2211 into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad-mojo-specs/+git/private:vbuilder-config-drive into ~launchpad/launchpad-mojo-specs/+git/private:vbuilder

[William Grant]

Review: Approve


-- 
https://code.launchpad.net/~cjwatson/launchpad-mojo-specs/+git/private/+merge/426647
Your team Launchpad code reviewers is subscribed to branch 
~launchpad/launchpad-mojo-specs/+git/private:vbuilder.


___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/meta-lp-deps/remove-silly-recommends into lp:meta-lp-deps

[William Grant]

Review: Approve


-- 
https://code.launchpad.net/~cjwatson/meta-lp-deps/remove-silly-recommends/+merge/421581
Your team Launchpad code reviewers is subscribed to branch lp:meta-lp-deps.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-cibuild-jobs into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/419156
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-cibuild-jobs into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad/+git/security:tighten-edit-dsp-permissions into launchpad:master

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/security/+merge/418937
Your team Launchpad code reviewers is subscribed to branch 
~cjwatson/launchpad/+git/security:tighten-edit-dsp-permissions.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-artifactory-publish-3 into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/418875
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-artifactory-publish-3 into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~lgp171188/launchpad:bugtask-changes-sql into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/417739
Your team Launchpad code reviewers is requested to review the proposed merge of 
~lgp171188/launchpad:bugtask-changes-sql into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-artifactory-publish into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/416727
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-artifactory-publish into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-artifactory-publish into launchpad:db-devel

[William Grant]

Review: Abstain db

Looks good, and the split looks sensible now. Thanks!
-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/416727
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-artifactory-publish into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-artifactory-publish into launchpad:db-devel

[William Grant]

Review: Needs Fixing db



Diff comments:

> diff --git a/database/schema/patch-2210-44-0.sql 
> b/database/schema/patch-2210-44-0.sql
> new file mode 100644
> index 000..bf5e78c
> --- /dev/null
> +++ b/database/schema/patch-2210-44-0.sql
> @@ -0,0 +1,106 @@
> +-- Copyright 2022 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +-- STEP 1, COLD
> +
> +ALTER TABLE Archive
> +-- 0 == LOCAL
> +ADD COLUMN publishing_method integer DEFAULT 0,
> +-- 0 == DEBIAN
> +ADD COLUMN index_format integer DEFAULT 0;

It's not just the index format that differs, since e.g. the pool structure is 
rather apt-specific. repository_format or repository_type instead?

> +
> +ALTER TABLE SourcePackageRelease
> +ADD COLUMN ci_build integer REFERENCES CIBuild,
> +ADD CONSTRAINT at_most_one_build CHECK (
> +null_count(ARRAY[sourcepackage_recipe_build, ci_build]) >= 1),

This will likely take approximately a very long time.

> +ALTER COLUMN component DROP NOT NULL,
> +ALTER COLUMN section DROP NOT NULL,
> +ALTER COLUMN urgency DROP NOT NULL,
> +ALTER COLUMN dsc_format DROP NOT NULL,
> +ADD CONSTRAINT debian_columns CHECK (
> +-- 1 == DPKG
> +format != 1
> +OR (component IS NOT NULL
> +AND section IS NOT NULL
> +AND urgency IS NOT NULL
> +AND dsc_format IS NOT NULL)),

Also roughly eternal.

> +-- This is always non-NULL for Debian-format source packages, but it's
> +-- not particularly important to constrain this at the DB level.
> +ALTER COLUMN maintainer DROP NOT NULL;
> +
> +ALTER TABLE SourcePackagePublishingHistory
> +ADD COLUMN format integer,
> +ALTER COLUMN component DROP NOT NULL,
> +ALTER COLUMN section DROP NOT NULL,
> +ADD CONSTRAINT debian_columns CHECK (
> +(format IS NOT NULL
> +-- 1 == DPKG
> +AND format != 1)

COALSECE(format, 1) != 1?

> +OR (component IS NOT NULL
> +AND section IS NOT NULL)),
> +ADD COLUMN channel jsonb;

Should this also be constrained to prevent channel from being set for 
format==DPKG?

Also likely slow.

> +
> +ALTER TABLE BinaryPackageRelease
> +ADD COLUMN ci_build integer REFERENCES CIBuild,
> +ALTER COLUMN build DROP NOT NULL,
> +ADD CONSTRAINT one_build CHECK (null_count(ARRAY[build, ci_build]) = 1),
> +ALTER COLUMN component DROP NOT NULL,
> +ALTER COLUMN section DROP NOT NULL,
> +ALTER COLUMN priority DROP NOT NULL,
> +ADD CONSTRAINT debian_columns CHECK (
> +-- 1 == DEB, 2 == UDEB, 5 == DDEB
> +binpackageformat NOT IN (1, 2, 5)
> +OR (component IS NOT NULL
> +AND section IS NOT NULL
> +AND priority IS NOT NULL));
> +
> +ALTER TABLE BinaryPackagePublishingHistory
> +ADD COLUMN binarypackageformat integer,
> +ALTER COLUMN component DROP NOT NULL,
> +ALTER COLUMN section DROP NOT NULL,
> +ALTER COLUMN priority DROP NOT NULL,
> +ADD CONSTRAINT debian_columns CHECK (
> +(binarypackageformat IS NOT NULL
> +-- 1 == DEB, 2 == UDEB, 5 == DDEB
> +AND binarypackageformat NOT IN (1, 2, 5))
> +OR (component IS NOT NULL
> +AND section IS NOT NULL
> +AND priority IS NOT NULL)),
> +ADD COLUMN channel jsonb;
> +
> +
> +-- Subsequent statements, to be executed live and in subsequent patches.
> +
> +/*
> +-- STEP 2, HOT
> +
> +CREATE INDEX sourcepackagerelease__ci_build__idx
> +ON SourcePackageRelease (ci_build);
> +
> +CREATE INDEX sourcepackagepublishinghistory__channel__idx
> +ON SourcePackagePublishingHistory (channel);

It doesn't seem massively likely that unscoped channel is interesting to query 
on. Inside an archive, maybe.

> +
> +CREATE UNIQUE INDEX binarypackagerelease__bpn__build__version__key
> +ON BinaryPackageRelease (
> +binarypackagename, COALESCE(build, ci_build), version);

I'm not quite sure what the purpose of 
binarypackagerelease_binarypackagename_key was in the first place, given that 
binarypackagerelease_build_name_uniq is stricter and it doesn't seem 
interesting for lookups.

Am I missing something, or should we drop it without replacement?

> +CREATE UNIQUE INDEX binarypackagerelease__build__bpn__key
> +ON BinaryPackageRelease (COALESCE(build, ci_build), binarypackagename);

COALESCEing two columns is almost certainly wrong in a unique constraint like 
this -- and wrong in a way we won't notice until it explodes by chance in a few 
years when build.id and cibuild.id happen to overlap for the same BPN. Consider 
something like (COALESCE(build, -1), COALESCE(ci_build, -1), binarypackagename).

It's also not strictly necessary that we coalesce at all. Two separate 
(optionally partial) indexes would work just fine.

> +CREATE INDEX binarypackagerelease__ci_build__idx
> +ON 

Re: [Launchpad-reviewers] [Merge] ~lgp171188/launchpad:lock-status-not-nullable into launchpad:db-devel

[William Grant]

Review: Approve db

As discussed on MM, this is going to take several seconds to scan the table, 
but that's acceptable downtime.
-- 
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/415259
Your team Launchpad code reviewers is requested to review the proposed merge of 
~lgp171188/launchpad:lock-status-not-nullable into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-private-distributions into launchpad:db-devel

[William Grant]

Review: Approve db

(Contention is impossible in cold patches, since all other DB connections are 
terminated beforehand. The issue with cold patches is just plain speed, which 
is indeed fine here. But it has to be a cold patch since most forms of `ALTER 
TABLE` take an `ACCESS EXCLUSIVE` lock on the involved table, which blocks even 
reads.)

Diff comments:

> diff --git a/database/schema/patch-2210-41-0.sql 
> b/database/schema/patch-2210-41-0.sql
> new file mode 100644
> index 000..d0d84a5
> --- /dev/null
> +++ b/database/schema/patch-2210-41-0.sql
> @@ -0,0 +1,34 @@
> +-- Copyright 2022 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +ALTER TABLE Distribution
> +ADD COLUMN branch_sharing_policy integer DEFAULT 1 NOT NULL,
> +ADD COLUMN bug_sharing_policy integer DEFAULT 1 NOT NULL,
> +ADD COLUMN specification_sharing_policy integer DEFAULT 1 NOT NULL,
> +ADD COLUMN information_type integer DEFAULT 1 NOT NULL,
> +ADD COLUMN access_policies integer[],
> +ADD CONSTRAINT distribution__valid_information_type CHECK (
> +information_type = ANY(ARRAY[1, 5, 6]));
> +
> +COMMENT ON COLUMN Distribution.branch_sharing_policy IS 'Sharing policy for 
> this distribution''s branches.';
> +COMMENT ON COLUMN Distribution.bug_sharing_policy IS 'Sharing policy for 
> this distribution''s bugs.';
> +COMMENT ON COLUMN Distribution.specification_sharing_policy IS 'Sharing 
> policy for this distribution''s specifications.';
> +COMMENT ON COLUMN Distribution.information_type IS 'Enum describing what 
> type of information is stored, such as type of private or security related 
> data, and used to determine how to apply an access policy.';
> +COMMENT ON COLUMN Distribution.access_policies IS 'Cache of AccessPolicy.ids 
> that convey launchpad.LimitedView.';
> +
> +ALTER TABLE CommercialSubscription
> +ADD COLUMN distribution integer REFERENCES distribution,
> +ALTER COLUMN product DROP NOT NULL,
> +ADD CONSTRAINT one_pillar CHECK (null_count(ARRAY[product, 
> distribution]) = 1);
> +
> +DROP INDEX commercialsubscription__product__idx;
> +CREATE INDEX commercialsubscription__product__idx
> +ON CommercialSubscription (product) WHERE product IS NOT NULL;
> +CREATE INDEX commercialsubscription__distribution__idx
> +ON CommercialSubscription (distribution) WHERE distribution IS NOT NULL;

commercialsubscription(product) is unique today, though the DB doesn't enforce 
it. Maybe we want to fix that here.

> +
> +COMMENT ON COLUMN CommercialSubscription.distribution IS 'The distribution 
> this subscription enables.';
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 41, 0);


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/415113
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-private-distributions into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-karmacache-id-bigint-3 into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/414933
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-karmacache-id-bigint-3 into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-ci-build into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/414172
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-ci-build into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-karmacache-id-bigint-1 into launchpad:db-devel

[William Grant]

Review: Approve db

TIL SQL supports C-style block comments.
-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/414058
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-karmacache-id-bigint-1 into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~ilasc/launchpad:add-revision-status-report into launchpad:db-devel

[William Grant]

Is there some way we can/should track who created the result?

Diff comments:

> diff --git a/database/schema/patch-2210-37-0.sql 
> b/database/schema/patch-2210-37-0.sql
> new file mode 100644
> index 000..ad34840
> --- /dev/null
> +++ b/database/schema/patch-2210-37-0.sql
> @@ -0,0 +1,51 @@
> +-- Copyright 2021 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +CREATE TABLE RevisionStatusArtifact (
> +id serial PRIMARY KEY,
> +library_file integer REFERENCES libraryfilealias
> +);
> +
> +COMMENT ON TABLE RevisionStatusArtifact IS 'A thin wrapper around 
> LibraryFileAlias.';
> +COMMENT ON COLUMN RevisionStatusArtifact.library_file IS 'Reference to 
> LibraryFileAlias.';
> +
> +CREATE TABLE RevisionStatusReport (
> +id serial PRIMARY KEY,
> +name text NOT NULL,
> +status INTEGER NOT NULL,
> +link text,

Something like external_url, maybe?

> +description text,
> +git_repository integer REFERENCES gitrepository NOT NULL,
> +commit_sha1 character(40) NOT NULL,

It seems like (git_repository, commit_sha1, name) are the main lookup key for 
the table. Can you move them to the start, right after the PK?

> +result integer,
> +date_started timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT 
> TIME ZONE 'UTC') NOT NULL,

Would it be useful to have a separate date_created?

> +date_finished timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT 
> TIME ZONE 'UTC') NOT NULL,

Does it make sense to default date_finished?

> +log integer

Should this exist now that RevisionStatusArtifact does?

> +);
> +
> +COMMENT ON TABLE RevisionStatusReport IS 'This table links the submitted 
> reports to the target git references.';
> +COMMENT ON COLUMN RevisionStatusReport.name IS 'Name of the report.';
> +COMMENT ON COLUMN RevisionStatusReport.status IS 'One of the RevisionStatus 
> enum values: Queued, Started, Completed, FailedToStart.';
> +COMMENT ON COLUMN RevisionStatusReport.link IS 'External URL to view result 
> of report.';
> +COMMENT ON COLUMN RevisionStatusReport.description IS 'Text description of 
> the result.';
> +COMMENT ON COLUMN RevisionStatusReport.git_repository IS 'Reference to the 
> GitRepository.';
> +COMMENT ON COLUMN RevisionStatusReport.commit_sha1 IS 'The commit sha1 for 
> the report.';
> +COMMENT ON COLUMN RevisionStatusReport.result IS 'One of the 
> RevisionStatusResult enum values: Success, Failed, Skipped, Cancelled.';
> +COMMENT ON COLUMN RevisionStatusReport.date_started IS 'DateTime that report 
> was started.';
> +COMMENT ON COLUMN RevisionStatusReport.date_finished IS 'DateTime that 
> report was completed.';
> +COMMENT ON COLUMN RevisionStatusReport.log IS 'Reference to a 
> RevisionStatusArtifact.';
> +
> +CREATE INDEX revision_status_report__git_repository__idx
> +ON RevisionStatusReport (git_repository);
> +
> +ALTER TABLE RevisionStatusArtifact ADD COLUMN revision_status integer 
> REFERENCES RevisionStatusReport;
> +COMMENT ON COLUMN RevisionStatusArtifact.revision_status IS 'Reference to 
> RevisionStatusReport.';

If you reorder the CREATE TABLEs you can just have this as "revision_status 
integer REFERENCES RevisionStatusReport" in the original definition.

> +
> +CREATE INDEX revision_status_artifact__library_file__idx
> +ON RevisionStatusArtifact (library_file);
> +CREATE INDEX revision_status_artifact__revision_status__idx
> +ON RevisionStatusArtifact (revision_status);
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 37, 0);


-- 
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/409900
Your team Launchpad code reviewers is requested to review the proposed merge of 
~ilasc/launchpad:add-revision-status-report into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-access-token into launchpad:db-devel

[William Grant]

Review: Approve db



Diff comments:

> diff --git a/database/schema/patch-2210-36-0.sql 
> b/database/schema/patch-2210-36-0.sql
> new file mode 100644
> index 000..fa08939
> --- /dev/null
> +++ b/database/schema/patch-2210-36-0.sql
> @@ -0,0 +1,38 @@
> +-- Copyright 2021 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +CREATE TABLE AccessToken (
> +id serial PRIMARY KEY,
> +date_created timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT 
> TIME ZONE 'UTC') NOT NULL,
> +token_sha256 text NOT NULL,
> +owner integer NOT NULL REFERENCES person,
> +description text NOT NULL,
> +git_repository integer REFERENCES gitrepository NOT NULL,
> +scopes jsonb NOT NULL,
> +date_last_used timestamp without time zone,

Updating this every time may well result in awful locking issues. We might want 
to use `SKIP LOCKED` for the first time.

SessionData.last_accessed somewhat gets away with it because it's a separate 
DB, only the first set of requests every few minutes trigger it, and it's 
relatively uncommon for there to be lots of concurrent webapp requests from one 
browser.

> +date_expires timestamp without time zone

Is revocation implemented just by setting this to now?

> +);
> +
> +COMMENT ON TABLE AccessToken IS 'A personal access token for the webservice 
> API.';
> +COMMENT ON COLUMN AccessToken.date_created IS 'When the token was created.';
> +COMMENT ON COLUMN AccessToken.token_sha256 IS 'SHA-256 hash of the secret 
> token.';
> +COMMENT ON COLUMN AccessToken.owner IS 'The person who created the token.';
> +COMMENT ON COLUMN AccessToken.description IS 'A short description of the 
> token''s purpose.';
> +COMMENT ON COLUMN AccessToken.git_repository IS 'The Git repository for 
> which the token was issued.';
> +COMMENT ON COLUMN AccessToken.scopes IS 'A list of scopes granted by the 
> token.';
> +COMMENT ON COLUMN AccessToken.date_last_used IS 'When the token was last 
> used.';
> +COMMENT ON COLUMN AccessToken.date_expires IS 'When the token should 
> expire.';
> +
> +CREATE UNIQUE INDEX accesstoken__token_sha256__key
> +ON AccessToken (token_sha256);
> +CREATE INDEX accesstoken__owner__idx
> +ON AccessToken (owner);
> +CREATE INDEX accesstoken__git_repository__idx
> +ON AccessToken (git_repository);
> +CREATE INDEX accesstoken__date_expires__idx
> +ON AccessToken (date_expires)
> +WHERE date_expires IS NOT NULL;
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 36, 0);


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/409463
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-access-token into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-charm-base into launchpad:db-devel

[William Grant]

Review: Approve db



Diff comments:

> diff --git a/database/schema/patch-2210-33-2.sql 
> b/database/schema/patch-2210-33-2.sql
> new file mode 100644
> index 000..ff60ec6
> --- /dev/null
> +++ b/database/schema/patch-2210-33-2.sql
> @@ -0,0 +1,33 @@
> +-- Copyright 2021 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +CREATE TABLE CharmBase (
> +id serial PRIMARY KEY,
> +date_created timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT 
> TIME ZONE 'UTC') NOT NULL,
> +registrant integer NOT NULL REFERENCES person,
> +distro_series integer NOT NULL REFERENCES distroseries,
> +build_channels text NOT NULL

Unlike in SnapBase, it isn't clear from the naming that this contains snap 
channels for a charm build.

Also, should this still be text now that we're on a modern postgres?

> +);
> +
> +CREATE INDEX charmbase__registrant__idx ON CharmBase (registrant);
> +CREATE UNIQUE INDEX charmbase__distro_series__key ON CharmBase 
> (distro_series);
> +
> +COMMENT ON TABLE CharmBase IS 'A base for charms.';
> +COMMENT ON COLUMN CharmBase.date_created IS 'The date on which this base was 
> created in Launchpad.';
> +COMMENT ON COLUMN CharmBase.registrant IS 'The user who registered this 
> base.';
> +COMMENT ON COLUMN CharmBase.distro_series IS 'The distro series for this 
> base.';
> +COMMENT ON COLUMN CharmBase.build_channels IS 'A dictionary mapping snap 
> names to channels to use when building charm recipes that specify this base.';
> +
> +CREATE TABLE CharmBaseArch (
> +charm_base integer NOT NULL REFERENCES charmbase,
> +processor integer NOT NULL REFERENCES processor,
> +PRIMARY KEY (charm_base, processor)
> +);
> +
> +COMMENT ON TABLE CharmBaseArch IS 'The architectures that a charm base 
> supports.';
> +COMMENT ON COLUMN CharmBaseArch.charm_base IS 'The charm base for which a 
> supported architecture is specified.';
> +COMMENT ON COLUMN CharmBaseArch.processor IS 'A supported architecture for 
> this charm base.';
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 33, 2);


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/409007
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-charm-base into launchpad:db-devel.


___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-stale-snap-charm-recipes into launchpad:master

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/408996
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-stale-snap-charm-recipes 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

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:comment-editing-db-patch into launchpad:db-devel

[William Grant]

Review: Approve db

Thanks, just a couple comments.

Diff comments:

> diff --git a/database/schema/patch-2210-31-0.sql 
> b/database/schema/patch-2210-31-0.sql
> new file mode 100644
> index 000..3a15c86
> --- /dev/null
> +++ b/database/schema/patch-2210-31-0.sql
> @@ -0,0 +1,47 @@
> +-- Copyright 2021 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +ALTER TABLE Message
> +ADD COLUMN date_deleted timestamp without time zone,
> +ADD COLUMN date_last_edited timestamp without time zone;
> +
> +CREATE TABLE MessageRevision (
> +id serial PRIMARY KEY,
> +message integer NOT NULL REFERENCES Message,
> +subject text,
> +revision integer NOT NULL,

I somewhat disagree with Colin here, and would default to using something like 
(message, date_created, id) as the ordering, but revision doesn't complicate 
things by much.

But can you put it before subject, so the key columns are first?

> +date_created timestamp without time zone NOT NULL,
> +date_deleted timestamp without time zone
> +) WITH (fillfactor='100');
> +
> +CREATE UNIQUE INDEX messagerevision__message__revision__key
> +ON MessageRevision(message, revision);
> +
> +COMMENT ON TABLE MessageRevision IS 'Old versions of an edited Message';
> +COMMENT ON COLUMN MessageRevision.message
> +IS 'The current message of this revision';
> +COMMENT ON COLUMN MessageRevision.revision
> +IS 'The revision monotonic increasing number';
> +COMMENT ON COLUMN MessageRevision.date_created
> +IS 'When the original message was edited and created this revision';
> +COMMENT ON COLUMN MessageRevision.date_deleted
> +IS 'If this revision was deleted, when did that happen';
> +
> +
> +CREATE TABLE MessageRevisionChunk (
> +id serial PRIMARY KEY,
> +messagerevision integer NOT NULL REFERENCES MessageRevision,
> +sequence integer NOT NULL,
> +content text NOT NULL
> +) WITH (fillfactor='100');

Should this have some indexes? Like UNIQUE (messagerevision, sequence), to 
allow lookup and ensure uniqueness.

> +
> +COMMENT ON TABLE MessageRevisionChunk
> +IS 'Old chunks of a message when a revision was created for it';
> +COMMENT ON COLUMN MessageRevisionChunk.sequence
> +IS 'Order of this particular chunk';
> +COMMENT ON COLUMN MessageRevisionChunk.content
> +IS 'Text content for this chunk of the message.';
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 31, 0);


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/401976
Your team Launchpad code reviewers is subscribed to branch 
~pappacena/launchpad:comment-editing-db-patch.

___
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

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:oci-bug-indexes into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/393579
Your team Launchpad code reviewers is subscribed to branch 
~pappacena/launchpad:oci-bug-add-oci-columns.

___
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

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:oci-bug-indexes into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/393579
Your team Launchpad code reviewers is subscribed to branch 
~pappacena/launchpad:oci-bug-add-oci-columns.

___
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

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:ocirecipe-private-dbpatch into launchpad:db-devel

[William Grant]

Review: Approve db



Diff comments:

> diff --git a/database/schema/patch-2210-26-3.sql 
> b/database/schema/patch-2210-26-3.sql
> new file mode 100644
> index 000..54c78c7
> --- /dev/null
> +++ b/database/schema/patch-2210-26-3.sql
> @@ -0,0 +1,122 @@
> +-- Copyright 2021 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +-- OCIRecipe privacy model is based only on ownership, similarly to Archives.
> +ALTER TABLE OCIRecipe
> +ADD COLUMN information_type integer,
> +ADD COLUMN access_policy integer,
> +ADD COLUMN access_grants integer[];
> +
> +COMMENT ON COLUMN OCIRecipe.information_type IS
> +'Enum describing what type of information is stored, such as type of 
> private or security related data, and used to determine to apply an access 
> policy.';
> +
> +
> +CREATE TABLE OCIRecipeSubscription (
> +id serial PRIMARY KEY,
> +recipe integer NOT NULL REFERENCES OCIRecipe(id),
> +person integer NOT NULL REFERENCES Person(id),
> +date_created timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT 
> TIME ZONE 'UTC') NOT NULL,
> +subscribed_by integer NOT NULL REFERENCES Person(id)
> +);
> +
> +COMMENT ON TABLE OCIRecipeSubscription IS 'Person subscription for OCI 
> recipe.';
> +COMMENT ON COLUMN OCIRecipeSubscription.person IS
> +'The person who subscribed to the OCI recipe.';
> +COMMENT ON COLUMN OCIRecipeSubscription.recipe IS
> +'The OCI recipe to which the person subscribed.';
> +COMMENT ON COLUMN OCIRecipeSubscription.date_created IS
> +'When the subscription was created.';
> +COMMENT ON COLUMN OCIRecipeSubscription.subscribed_by IS
> +'The person performing the action of subscribing someone to the OCI 
> recipe.';
> +
> +CREATE UNIQUE INDEX ocirecipesubscription__person_recipe__key

s/person_recipe/recipe__person/

> +ON OCIRecipeSubscription(recipe, person);
> +
> +CREATE INDEX ocirecipesubscription__person__idx
> +ON OCIRecipeSubscription(person);
> +
> +CREATE INDEX ocirecipesubscription__subscribed_by__idx
> +ON OCIRecipeSubscription(subscribed_by);
> +
> +ALTER TABLE AccessArtifact
> +ADD COLUMN ocirecipe integer REFERENCES OCIRecipe;
> +
> +
> +ALTER TABLE AccessArtifact DROP CONSTRAINT has_artifact;
> +ALTER TABLE AccessArtifact
> +ADD CONSTRAINT has_artifact CHECK (
> +(null_count(ARRAY[bug, branch, gitrepository, snap, specification, 
> ocirecipe]) = 5)) NOT VALID;
> +
> +
> +CREATE OR REPLACE FUNCTION ocirecipe_denorm_access(ocirecipe_id integer)
> +RETURNS void LANGUAGE plpgsql AS
> +$$
> +DECLARE
> +info_type integer;
> +BEGIN
> +SELECT
> +-- information type: 1 = public
> +COALESCE(ocirecipe.information_type, 1)
> +INTO info_type
> +FROM ocirecipe WHERE id = ocirecipe_id;
> +
> +UPDATE OCIRecipe
> +SET access_policy = policies[1], access_grants = grants
> +FROM
> +build_access_cache(
> +(SELECT id FROM accessartifact WHERE ocirecipe = 
> ocirecipe_id),
> +info_type)
> +AS (policies integer[], grants integer[])
> +WHERE id = ocirecipe_id;
> +END;
> +$$;
> +
> +CREATE OR REPLACE FUNCTION accessartifact_denorm_to_artifacts(artifact_id 
> integer)
> +RETURNS void
> +LANGUAGE plpgsql
> +AS $$
> +DECLARE
> +artifact_row accessartifact%ROWTYPE;
> +BEGIN
> +SELECT * INTO artifact_row FROM accessartifact WHERE id = artifact_id;
> +IF artifact_row.bug IS NOT NULL THEN
> +PERFORM bug_flatten_access(artifact_row.bug);
> +END IF;
> +IF artifact_row.branch IS NOT NULL THEN
> +PERFORM branch_denorm_access(artifact_row.branch);
> +END IF;
> +IF artifact_row.gitrepository IS NOT NULL THEN
> +PERFORM gitrepository_denorm_access(artifact_row.gitrepository);
> +END IF;
> +IF artifact_row.snap IS NOT NULL THEN
> +PERFORM snap_denorm_access(artifact_row.snap);
> +END IF;
> +IF artifact_row.specification IS NOT NULL THEN
> +PERFORM specification_denorm_access(artifact_row.specification);
> +END IF;
> +IF artifact_row.ocirecipe IS NOT NULL THEN
> +PERFORM ocirecipe_denorm_access(artifact_row.ocirecipe);
> +END IF;
> +RETURN;
> +END;
> +$$;
> +
> +COMMENT ON FUNCTION accessartifact_denorm_to_artifacts(artifact_id integer) 
> IS
> +'Denormalize the policy access and artifact grants to bugs, branches, 
> git repositories, snaps, specifications and ocirecipe.';
> +
> +-- A trigger to handle ocirecipe.information_type changes.
> +CREATE OR REPLACE FUNCTION ocirecipe_maintain_access_cache_trig() RETURNS 
> trigger
> +LANGUAGE plpgsql AS $$
> +BEGIN
> +PERFORM ocirecipe_denorm_access(NEW.id);
> +RETURN NULL;
> +END;
> +$$;
> +
> +CREATE TRIGGER ocirecipe_maintain_access_cache
> +AFTER INSERT OR UPDATE OF information_type ON OCIRecipe
> +FOR EACH ROW EXECUTE 

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:ocirecipe-private-dbpatch-indexes into launchpad:master

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/399243
Your team Launchpad code reviewers is subscribed to branch 
~pappacena/launchpad:ocirecipe-private-dbpatch.

___
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

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:snap-pillar-db-indexes into launchpad:master

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/398702
Your team Launchpad code reviewers is subscribed to branch 
~pappacena/launchpad:snap-pillar-db.

___
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

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:snap-pillar-db-indexes into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/398361
Your team Launchpad code reviewers is subscribed to branch 
~pappacena/launchpad:snap-pillar-db.

___
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

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:snap-pillar-db into launchpad:db-devel

[William Grant]

Review: Approve db


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/397459
Your team Launchpad code reviewers is subscribed to branch 
~pappacena/launchpad:snap-pillar-db.

___
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

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:snap-pillar-db into launchpad:db-devel

[William Grant]

Review: Approve db

Thanks, just a couple of small things.

Diff comments:

> diff --git a/database/schema/patch-2210-26-1.sql 
> b/database/schema/patch-2210-26-1.sql
> new file mode 100644
> index 000..5511d91
> --- /dev/null
> +++ b/database/schema/patch-2210-26-1.sql
> @@ -0,0 +1,132 @@
> +-- Copyright 2021 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +ALTER TABLE Snap
> +ADD COLUMN information_type integer,
> +ADD COLUMN project integer REFERENCES product,

This should probably have an index.

> +ADD COLUMN access_policy integer,

Does this normally have an FK? access_grants obviously can't, but I don't 
remember if I bothered with access_policy.

> +ADD COLUMN access_grants integer[];
> +
> +COMMENT ON COLUMN Snap.private IS
> +'(DEPRECATED; use Snap.information_type) Whether or not this snap is 
> private.';
> +COMMENT ON COLUMN Snap.project IS
> +'The project which is the pillar for this Snap';
> +COMMENT ON COLUMN Snap.information_type IS
> +'Enum describing what type of information is stored, such as type of 
> private or security related data, and used to determine to apply an access 
> policy.';
> +
> +CREATE TABLE SnapSubscription (
> +id serial PRIMARY KEY,
> +person integer NOT NULL REFERENCES Person(id),
> +snap integer NOT NULL REFERENCES Snap(id),

Absolutely trivial, but I'd have snap before person.

> +date_created timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT 
> TIME ZONE 'UTC') NOT NULL,
> +subscribed_by integer NOT NULL REFERENCES Person(id)

This should have an index, or person merge is going to slowly get mysteriously 
slower.

> +);
> +
> +COMMENT ON TABLE SnapSubscription IS 'Person subscription for Snap recipes.';
> +COMMENT ON COLUMN SnapSubscription.person IS
> +'The person who subscribed to the Snap.';
> +COMMENT ON COLUMN SnapSubscription.snap IS
> +'The Snap recipe to which the person subscribed.';
> +COMMENT ON COLUMN SnapSubscription.date_created IS
> +'When the subscription was created.';
> +COMMENT ON COLUMN SnapSubscription.subscribed_by IS
> +'The person performing the action of subscribing someone to the Snap.';
> +
> +
> +CREATE UNIQUE INDEX snapsubscription__person_snap__key
> +ON SnapSubscription(snap, person);
> +
> +CREATE INDEX snapsubscription__person__idx
> +ON SnapSubscription(person);
> +
> +ALTER TABLE AccessArtifact
> +ADD COLUMN snap integer REFERENCES snap;
> +
> +CREATE UNIQUE INDEX accessartifact__snap__key
> +ON AccessArtifact(snap) WHERE snap IS NOT NULL;
> +
> +
> +ALTER TABLE AccessArtifact DROP CONSTRAINT has_artifact;
> +ALTER TABLE AccessArtifact
> +ADD CONSTRAINT has_artifact CHECK (
> +(null_count(ARRAY[bug, branch, gitrepository, snap, specification]) = 
> 4));

This is probably slow. We should benchmark, but may need to catalogue-hack 
(unless VALIDATE CONSTRAINT is fixed to no longer take a serious lock now).

> +
> +
> +CREATE OR REPLACE FUNCTION snap_denorm_access(snap_id integer)
> +RETURNS void LANGUAGE plpgsql AS
> +$$
> +DECLARE
> +info_type integer;
> +BEGIN
> +-- XXX pappacena 2021-002-12: Once we finish filling "information_type" 
> and

I know 2020 was weird, but I hope 2021 isn't that long.

> +-- deprecate the usage of "public" column at code level, we will be able 
> to
> +-- drop the "private" column usage here.
> +SELECT
> +CASE snap.information_type
> +WHEN NULL THEN
> +-- information type: 1 = public; 5 = proprietary
> +CASE WHEN snap.private THEN 5 ELSE 1 END
> +ELSE
> +snap.information_type
> +END
> +INTO info_type
> +FROM snap WHERE id = snap_id;

"CASE foo WHEN NULL THEN bar ELSE foo" can also be spelt "COALESCE(foo, bar)".

> +
> +UPDATE Snap
> +SET access_policy = policies[1], access_grants = grants
> +FROM
> +build_access_cache(
> +(SELECT id FROM accessartifact WHERE snap = snap_id),
> +info_type)
> +AS (policies integer[], grants integer[])
> +WHERE id = snap_id;
> +END;
> +$$;
> +
> +CREATE OR REPLACE FUNCTION accessartifact_denorm_to_artifacts(artifact_id 
> integer)
> +RETURNS void
> +LANGUAGE plpgsql
> +AS $$
> +DECLARE
> +artifact_row accessartifact%ROWTYPE;
> +BEGIN
> +SELECT * INTO artifact_row FROM accessartifact WHERE id = artifact_id;
> +IF artifact_row.bug IS NOT NULL THEN
> +PERFORM bug_flatten_access(artifact_row.bug);
> +END IF;
> +IF artifact_row.branch IS NOT NULL THEN
> +PERFORM branch_denorm_access(artifact_row.branch);
> +END IF;
> +IF artifact_row.gitrepository IS NOT NULL THEN
> +PERFORM gitrepository_denorm_access(artifact_row.gitrepository);
> +END IF;
> +IF artifact_row.snap IS NOT NULL 

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:oci-bug-indexes into launchpad:db-devel

[William Grant]

Diff comments:

> diff --git a/database/schema/patch-2210-22-1.sql 
> b/database/schema/patch-2210-22-1.sql
> new file mode 100644
> index 000..51550d6
> --- /dev/null
> +++ b/database/schema/patch-2210-22-1.sql
> @@ -0,0 +1,140 @@
> +-- Copyright 2020 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +-- Validate check constraint.
> +ALTER TABLE BugTask VALIDATE CONSTRAINT bugtask_assignment_checks;
> +
> +ALTER TABLE BugSummary VALIDATE CONSTRAINT bugtask_assignment_checks;
> +
> +-- BugTask indexes.
> +CREATE UNIQUE INDEX bugtask__ociproject__bug__idx
> +ON BugTask (ociproject, bug)
> +WHERE ociproject IS NOT NULL;
> +CREATE UNIQUE INDEX bugtask__ociprojectseries__bug__idx
> +ON BugTask (ociprojectseries, bug)
> +WHERE ociprojectseries IS NOT NULL;
> +
> +-- BugTaskFlat indexes.
> +CREATE INDEX bugtaskflat__ociproject__bug__idx
> +ON BugTaskFlat (ociproject, bug)
> +WHERE ociproject IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociproject__date_closed__bug__idx
> +ON BugTaskFlat (ociproject, date_closed, bug DESC)
> +WHERE ociproject IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociproject__date_last_updated__idx
> +ON BugTaskFlat (ociproject, date_last_updated)
> +WHERE ociproject IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociproject__datecreated__idx
> +ON BugTaskFlat (ociproject, datecreated)
> +WHERE ociproject IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociproject__heat__bug__idx
> +ON BugTaskFlat (ociproject, heat, bug DESC)
> +WHERE ociproject IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociproject__importance__bug__idx
> +ON BugTaskFlat (ociproject, importance, bug DESC)
> +WHERE ociproject IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociproject__latest_patch_uploaded__bug__idx
> +ON BugTaskFlat (ociproject, latest_patch_uploaded, bug DESC)
> +WHERE ociproject IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociproject__status__bug__idx
> +ON BugTaskFlat (ociproject, status, bug DESC)
> +WHERE ociproject IS NOT NULL;
> +
> +CREATE INDEX bugtaskflat__ociprojectseries__bug__idx
> +ON BugTaskFlat (ociprojectseries, bug)
> +WHERE ociprojectseries IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociprojectseries__date_closed__bug__idx
> +ON BugTaskFlat (ociprojectseries, date_closed, bug DESC)
> +WHERE ociprojectseries IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociprojectseries__date_last_updated__idx
> +ON BugTaskFlat (ociprojectseries, date_last_updated)
> +WHERE ociprojectseries IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociprojectseries__datecreated__idx
> +ON BugTaskFlat (ociprojectseries, datecreated)
> +WHERE ociprojectseries IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociprojectseries__heat__bug__idx
> +ON BugTaskFlat (ociprojectseries, heat, bug DESC)
> +WHERE ociprojectseries IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociprojectseries__importance__bug__idx
> +ON BugTaskFlat (ociprojectseries, importance, bug DESC)
> +WHERE ociprojectseries IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociprojectseries__latest_patch_uploaded__bug__idx
> +ON BugTaskFlat (ociprojectseries, latest_patch_uploaded, bug DESC)
> +WHERE ociprojectseries IS NOT NULL;
> +CREATE INDEX bugtaskflat__ociprojectseries__status__bug__idx
> +ON BugTaskFlat (ociprojectseries, status, bug DESC)
> +WHERE ociprojectseries IS NOT NULL;
> +
> +
> +-- BugSummary indexes.
> +CREATE INDEX bugsummary__ociproject__idx
> +ON BugSummary (ociproject)
> +WHERE ociproject IS NOT NULL;
> +CREATE INDEX bugsummary__ociprojectseries__idx
> +ON BugSummary (ociprojectseries)
> +WHERE ociprojectseries IS NOT NULL;
> +
> +
> +-- Replacing previously renamed indexes.
> +CREATE UNIQUE INDEX bugtask_distinct_sourcepackage_assignment
> +ON BugTask (
> +bug,
> +COALESCE(sourcepackagename, -1),
> +COALESCE(distroseries, -1),
> +COALESCE(distribution, -1)
> +)
> +WHERE
> +product IS NULL
> +AND productseries IS NULL
> +AND ociproject IS NULL
> +AND ociprojectseries IS NULL;

This change makes sense, though it's getting to a point where it might be 
clearer to replace this condition with "distribution IS NOT NULL OR 
distroseries IS NOT NULL".

Or possibly even merge bugtask__product__bug__key, 
bugtask__productseries__bug__key, bugtask_distinct_sourcepackage_assignment, 
bugtask__ociproject_bug__idx and bugtask__ociprojectseries__bug__idx into a 
single constraint like the start of bugsummary__unique.

> +DROP INDEX old__bugtask_distinct_sourcepackage_assignment;
> +
> +
> +CREATE UNIQUE INDEX bugtask__product__bug__key
> +ON BugTask (product, bug)
> +WHERE
> +product IS NOT NULL
> +AND ociproject IS NULL
> +AND ociprojectseries IS NULL;

This will cause the index to no longer be used to find all bugs in a 

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:oci-bug-add-oci-columns into launchpad:db-devel

[William Grant]

Review: Approve db

Thanks, just a couple of trivial fixes.

Diff comments:

> diff --git a/database/schema/patch-2210-22-0.sql 
> b/database/schema/patch-2210-22-0.sql
> new file mode 100644
> index 000..d7f808f
> --- /dev/null
> +++ b/database/schema/patch-2210-22-0.sql
> @@ -0,0 +1,486 @@
> +-- Copyright 2020 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +-- This patch can be applied "cold", in a "fast-downtime" way. It basically
> +-- adds the new OCI database columns, and rename current indexes so they can 
> be
> +-- removed and replaced with new ones that includes the new columns.
> +
> +ALTER TABLE BugTask
> +ADD COLUMN ociproject integer REFERENCES ociproject,
> +ADD COLUMN ociprojectseries integer REFERENCES ociprojectseries,
> +DROP CONSTRAINT bugtask_assignment_checks,
> +ADD CONSTRAINT bugtask_assignment_checks CHECK (
> +CASE
> +WHEN product IS NOT NULL THEN productseries IS NULL AND 
> distribution IS NULL AND distroseries IS NULL AND sourcepackagename IS NULL
> +WHEN productseries IS NOT NULL THEN distribution IS NULL AND 
> distroseries IS NULL AND sourcepackagename IS NULL AND ociproject IS NULL AND 
> ociprojectseries IS NULL
> +WHEN distribution IS NOT NULL THEN distroseries IS NULL
> +WHEN distroseries IS NOT NULL THEN ociproject IS NULL AND 
> ociprojectseries IS NULL
> +WHEN ociproject IS NOT NULL THEN ociprojectseries IS NULL AND 
> (distribution IS NOT NULL OR product IS NOT NULL)
> +WHEN ociprojectseries IS NOT NULL THEN ociproject IS NULL AND 
> (distribution IS NOT NULL OR product IS NOT NULL)

This permits rows with both sourcepackagename and either ociproject or 
ociprojectseries set, which I don't think is intentional.

> +ELSE false
> +END) NOT VALID;
> +
> +ALTER INDEX bugtask_distinct_sourcepackage_assignment
> +RENAME TO old__bugtask_distinct_sourcepackage_assignment;
> +
> +ALTER INDEX bugtask__product__bug__key
> +RENAME TO old__bugtask__product__bug__key;
> +
> +ALTER TABLE BugTaskFlat
> +ADD COLUMN ociproject integer,
> +ADD COLUMN ociprojectseries integer;
> +
> +ALTER TABLE BugSummary
> +ADD COLUMN ociproject integer REFERENCES ociproject,
> +ADD COLUMN ociprojectseries integer REFERENCES ociprojectseries,
> +DROP CONSTRAINT bugtask_assignment_checks,
> +ADD CONSTRAINT bugtask_assignment_checks CHECK (
> +CASE
> +WHEN product IS NOT NULL THEN productseries IS NULL AND 
> distribution IS NULL AND distroseries IS NULL AND sourcepackagename IS NULL
> +WHEN productseries IS NOT NULL THEN distribution IS NULL AND 
> distroseries IS NULL AND sourcepackagename IS NULL AND ociproject IS NULL AND 
> ociprojectseries IS NULL
> +WHEN distribution IS NOT NULL THEN distroseries IS NULL
> +WHEN distroseries IS NOT NULL THEN ociproject IS NULL AND 
> ociprojectseries IS NULL
> +WHEN ociproject IS NOT NULL THEN ociprojectseries IS NULL AND 
> (distribution IS NOT NULL OR product IS NOT NULL)
> +WHEN ociprojectseries IS NOT NULL THEN ociproject IS NULL AND 
> (distribution IS NOT NULL OR product IS NOT NULL)
> +ELSE false
> +END) NOT VALID;
> +
> +ALTER INDEX bugsummary__unique
> +RENAME TO old__bugsummary__unique;
> +
> +ALTER TABLE BugSummaryJournal
> +ADD COLUMN ociproject integer,
> +ADD COLUMN ociprojectseries integer;
> +
> +-- Functions
> +
> +CREATE OR REPLACE FUNCTION bugtask_flatten(task_id integer, check_only 
> boolean)
> +RETURNS boolean
> +LANGUAGE plpgsql SECURITY DEFINER SET search_path = public
> +AS $$
> +DECLARE
> +bug_row Bug%ROWTYPE;
> +task_row BugTask%ROWTYPE;
> +old_flat_row BugTaskFlat%ROWTYPE;
> +new_flat_row BugTaskFlat%ROWTYPE;
> +_product_active boolean;
> +_access_policies integer[];
> +_access_grants integer[];
> +BEGIN
> +-- This is the master function to update BugTaskFlat, but there are
> +-- maintenance triggers and jobs on the involved tables that update
> +-- it directly. Any changes here probably require a corresponding
> +-- change in other trigger functions.
> +
> +SELECT * INTO task_row FROM BugTask WHERE id = task_id;
> +SELECT * INTO old_flat_row FROM BugTaskFlat WHERE bugtask = task_id;
> +
> +-- If the task doesn't exist, ensure that there's no flat row.
> +IF task_row.id IS NULL THEN
> +IF old_flat_row.bugtask IS NOT NULL THEN
> +IF NOT check_only THEN
> +DELETE FROM BugTaskFlat WHERE bugtask = task_id;
> +END IF;
> +RETURN FALSE;
> +ELSE
> +RETURN TRUE;
> +END IF;
> +END IF;
> +
> +SELECT * FROM bug INTO bug_row WHERE id = task_row.bug;
> +
> +-- If it's a product(series) task, we must 

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:stop-ppa-key-propagation into launchpad:master

[William Grant]

https://bugs.launchpad.net/launchpad/+bug/357177 was where the behaviour was 
implemented, which links to 
https://lists.launchpad.net/launchpad-users/msg04943.html.

There were in the past concerns about keyserver pollution, I believe. PPAs are 
now often pretty ephemeral, and frequently created and deleted, but all the 
keys will stay around forever, filling up name-based key search results. It 
would also reveal the name of private PPAs.

I don't think we should change the behaviour until we stop using the main 
keyserver network for PPA keys.
-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/392627
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:stop-ppa-key-propagation 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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:archive-file-history into launchpad:master

[William Grant]

I found the new _updateByHash structure somewhat difficult to understand, and 
tried a different approach in 
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/390811. Not 
nearly landable, but I think the flow is much clearer, and I'd be interested in 
your thoughts.

Diff comments:

> diff --git a/lib/lp/archivepublisher/publishing.py 
> b/lib/lp/archivepublisher/publishing.py
> index 5908ccc..4735b89 100644
> --- a/lib/lp/archivepublisher/publishing.py
> +++ b/lib/lp/archivepublisher/publishing.py
> @@ -1084,33 +1083,54 @@ class Publisher(object):
>  real_path = os.path.join(suite_dir, real_name)
>  current_files[path] = (
>  int(current_entry["size"]), current_entry["sha256"], 
> real_path)
> +
> +# Gather information on entries currently in the database.  Ensure
> +# that we know about all the relevant by-hash directory trees before
> +# doing any removals so that we can prune them properly later, and
> +# work out which condemned files should be reprieved due to the
> +# paths in question having their previous content again.
> +reprieved_files = defaultdict(list)
>  uncondemned_files = set()

It took me far too long to work out that uncondemned_files has been completely 
repurposed: it previously contained the files that were to be uncondemned (and 
so were to have their condemnation rescinded), but now is those files that are 
*currently* uncondemned (and so may render the rescinding of another file's 
condemnation, now in the set called reprieved_files, unnecessary). This 
explains why I got rapidly confused by the changes when I looked at this a few 
times over the last couple of years!

>  for db_file in archive_file_set.getByArchive(
> -self.archive, container=container, only_condemned=True,
> -eager_load=True):
> -stripped_path = strip_dists(db_file.path)
> -if stripped_path in current_files:
> -current_sha256 = current_files[stripped_path][1]
> -if db_file.library_file.content.sha256 == current_sha256:
> -uncondemned_files.add(db_file)
> -if uncondemned_files:
> -for container, path, sha256 in 
> archive_file_set.unscheduleDeletion(
> -uncondemned_files):
> +self.archive, container=container, eager_load=True):
> +
> by_hashes.registerChild(os.path.dirname(strip_dists(db_file.path)))
> +file_key = (db_file.path, db_file.library_file.content.sha256)
> +if db_file.scheduled_deletion_date is None:
> +uncondemned_files.add(file_key)
> +else:
> +stripped_path = strip_dists(db_file.path)
> +if stripped_path in current_files:
> +current_sha256 = current_files[stripped_path][1]
> +if db_file.library_file.content.sha256 == current_sha256:
> +reprieved_files[file_key].append(db_file)
> +
> +# We may already have uncondemned entries with the same path and
> +# content as condemned entries that we were about to reprieve; if
> +# so, there's no need to reprieve them.
> +for file_key in uncondemned_files:
> +reprieved_files.pop(file_key, None)
> +
> +# Make sure nothing in the current Release file is condemned.
> +if reprieved_files:
> +reprieved_files_flat = set(
> +chain.from_iterable(reprieved_files.values()))
> +archive_file_set.unscheduleDeletion(reprieved_files_flat)
> +for db_file in reprieved_files_flat:
>  self.log.debug(
>  "by-hash: Unscheduled %s for %s in %s for deletion" % (
> -sha256, path, container))
> +db_file.library_file.content.sha256, db_file.path,
> +db_file.container))

Is separate reprieval interesting now that it just creates a new row anyway? It 
makes already complicated code significantly more involved, just to avoid 
either uploading a file again.

>  
>  # Remove any condemned files from the database whose stay of
>  # execution has elapsed.  We ensure that we know about all the
>  # relevant by-hash directory trees before doing any removals so that
>  # we can prune them properly later.
> -for db_file in archive_file_set.getByArchive(
> -self.archive, container=container):
> -
> by_hashes.registerChild(os.path.dirname(strip_dists(db_file.path)))
>  for container, path, sha256 in archive_file_set.reap(
>  self.archive, container=container):
> -self.log.debug(
> -"by-hash: Deleted %s for %s in %s" % (sha256, path, 
> container))
> +if (path, sha256) not in uncondemned_files:
> +

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:create-mp-refs into launchpad:master

[William Grant]

On 12/9/20 12:38 am, Thiago F. Pappacena wrote:
> wgrant, good point.
> 
> When a user opens a MP#1 targeting RepositoryX's master, for example, 
> RepositoryX itself will have a read-only ref called `refs/merge/1/head`. The 
> idea is that whomever is responsible for RepositoryX will have an easier way 
> to pull locally the changes introduced by MP#1.
> 
> Let's assume a RepositoryX is private. In theory, nothing changes for the 
> user opening the MP#1: the privacy checks and requirements to actually open a 
> new MP targeting RepositoryX are still the same. 
> 
> The only extra security check introduced on RepositoryX will be on Turnip 
> side, to block pushes to `refs/merge/...` namespace: 
> https://code.launchpad.net/~pappacena/turnip/+git/turnip/+merge/390620.
> 
> Do you see any specific privacy problem with this scenario?

The problem arises when the *source* repository is private. Consider,
for example, a security fix MP: a user can only view an MP if they can
see both the source and target branches. But this will let anyone who
can see the target repository examine the code in the MP from a
potentially invisible private branch.

-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/390581
Your team Launchpad code reviewers is requested to review the proposed merge of 
~pappacena/launchpad:create-mp-refs 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

Re: [Launchpad-reviewers] [Merge] ~pappacena/launchpad:create-mp-refs into launchpad:master

[William Grant]

How does this interact with privacy?
-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/390581
Your team Launchpad code reviewers is requested to review the proposed merge of 
~pappacena/launchpad:create-mp-refs 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

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:preflight-10s into launchpad:master

[William Grant]

William Grant has proposed merging ~wgrant/launchpad:preflight-10s into 
launchpad:master.

Commit message:
Wait longer in preflight.py for connections to die

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/380708

Despite the comment saying it could wait for 10s, it in fact only waited
for up to 2s. This has for some time not been reliably sufficient, as
a rerun of full-update has often been necessary.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:preflight-10s into launchpad:master.
diff --git a/database/schema/preflight.py b/database/schema/preflight.py
index 6bf1abb..0a4053c 100755
--- a/database/schema/preflight.py
+++ b/database/schema/preflight.py
@@ -337,9 +337,9 @@ class KillConnectionsPreflight(DatabasePreflight):
 
 System users are defined by SYSTEM_USERS.
 """
-# We keep trying to terminate connections every 0.5 seconds for
+# We keep trying to terminate connections every 0.1 seconds for
 # up to 10 seconds.
-num_tries = 20
+num_tries = 100
 seconds_to_pause = 0.1
 if self.replication_paused:
 nodes = set([self.lpmain_master_node])
___
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

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:security.py-numeric-usernames into launchpad:master

[William Grant]

William Grant has proposed merging 
~wgrant/launchpad:security.py-numeric-usernames into launchpad:master.

Commit message:
Fix security.py to not crash on a role name with digits

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/380297

PostgreSQL's aclitem putid emits role names unquoted in safe cases, but
security.py's regex didn't use the same safe set so failed to parse
ACLs involving usernames like "abc123".
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:security.py-numeric-usernames into launchpad:master.
diff --git a/database/schema/security.py b/database/schema/security.py
index 4e0c917..967790d 100755
--- a/database/schema/security.py
+++ b/database/schema/security.py
@@ -49,7 +49,10 @@ POSTGRES_ACL_MAP = {
 'T': 'TEMPORARY',
 }
 
-QUOTED_STRING_RE = '(?:([a-z_]+)|"([^"]*(?:""[^"]*)*)")?'
+# PostgreSQL's putid emits an unquoted string if every character in the role
+# name isalnum or is _. Otherwise the name is enclosed in double quotes, and
+# any embedded double quotes are doubled.
+QUOTED_STRING_RE = '(?:([A-Za-z0-9_]+)|"([^"]*(?:""[^"]*)*)")?'
 ACLITEM_RE = re.compile('^%(qs)s=([\w*]*)/%(qs)s$' % {'qs': QUOTED_STRING_RE})
 
 
___
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

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:pg10-stattuple-perms into launchpad:master

[William Grant]

William Grant has proposed merging ~wgrant/launchpad:pg10-stattuple-perms into 
launchpad:master.

Commit message:
Update security.cfg for PostgreSQL 10's pgstattuple

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/380003

Just eliminates security.py warnings.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:pg10-stattuple-perms into launchpad:master.
diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 5894f34..f38c416 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -62,9 +62,14 @@ public.min(debversion) = EXECUTE
 public.name_blacklist_match(text, integer) = EXECUTE
 public.null_count(anyarray)= EXECUTE
 public.person_sort_key(text, text) = EXECUTE
-public.pgstattuple(oid)=
-public.pgstattuple(text)   =
+public.pgstatindex(regclass)   =
+public.pgstatginindex(regclass)=
+public.pgstathashindex(regclass)   =
 public.pgstatindex(text)   =
+public.pgstattuple(regclass)   =
+public.pgstattuple_approx(regclass)=
+public.pgstattuple(text)   =
+public.pg_relpages(regclass)   =
 public.pg_relpages(text)   =
 public.pillarname  = SELECT
 public.replication_lag()   = EXECUTE
___
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

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:bprdc-ios into launchpad:master

[William Grant]

William Grant has proposed merging ~wgrant/launchpad:bprdc-ios into 
launchpad:master.

Commit message:
Index BinaryPackageReleaseDownloadCount for index-only scans

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/379942

Creating an index on binarypackagereleasedownloadcount (archive, 
binary_package_release, day, count) allows 
BinaryPackagePublishingHistory.getDailyDownloadTotals and 
Archive.getPackageDownloadCount to execute as index-only scans and not touch 
the heap at all. One sample query drops from 2789 page reads to 177.

Should be applied hot.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:bprdc-ios into launchpad:master.
diff --git a/database/schema/patch-2210-01-4.sql b/database/schema/patch-2210-01-4.sql
new file mode 100644
index 000..4ec5185
--- /dev/null
+++ b/database/schema/patch-2210-01-4.sql
@@ -0,0 +1,8 @@
+-- Copyright 2020 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+
+CREATE INDEX binarypackagereleasedownloadcount__index_only_scan__idx
+ON binarypackagereleasedownloadcount (
+	archive, binary_package_release, day, count);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 01, 4);
___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/meta-lp-deps/python-tdb into lp:meta-lp-deps

[William Grant]

Review: Approve


-- 
https://code.launchpad.net/~cjwatson/meta-lp-deps/python-tdb/+merge/378925
Your team Launchpad code reviewers is subscribed to branch lp:meta-lp-deps.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/meta-lp-deps/remove-python-subversion into lp:meta-lp-deps

[William Grant]

Review: Approve


-- 
https://code.launchpad.net/~cjwatson/meta-lp-deps/remove-python-subversion/+merge/378802
Your team Launchpad code reviewers is subscribed to branch lp:meta-lp-deps.

___
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

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-bugsummary-statement-triggers into launchpad:db-devel

[William Grant]

During the review I wanted to see if a few changes in approach were viable, but 
ended up going further than expected and wound up with a solution that is 
structured a bit differently, and doesn't use any dynamic queries at all: 
https://paste.ubuntu.com/p/SQzFYMBBqZ/

Thoughts on that?

Diff comments:

> diff --git a/database/schema/patch-2210-06-0.sql 
> b/database/schema/patch-2210-06-0.sql
> new file mode 100644
> index 000..b411d69
> --- /dev/null
> +++ b/database/schema/patch-2210-06-0.sql
> @@ -0,0 +1,251 @@
> +-- Copyright 2019 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +-- Rewrite the BugSummaryJournal maintenance triggers to make use of the new
> +-- transition tables provided to AFTER ... FOR EACH STATEMENT triggers as of
> +-- PostgreSQL 10.  Instead of using row-level triggers which accumulate
> +-- changes in a temporary table and flush it into the journal, we now write
> +-- directly to the journal at the end of each statement.
> +
> +DROP TRIGGER bugtaskflat_maintain_bug_summary ON bugtaskflat;
> +DROP TRIGGER bugtag_maintain_bug_summary_before_trigger ON bugtag;
> +DROP TRIGGER bugtag_maintain_bug_summary_after_trigger ON bugtag;
> +
> +-- Similar to the previous bugsummary_tags, but returns an array of tags on
> +-- the given bug plus NULL; this can be passed to other functions, and is
> +-- suitable for constructing a set of rows to insert into BugSummaryJournal
> +-- when handling changes to BugTaskFlat.
> +CREATE OR REPLACE FUNCTION bugsummary_tag_names(bug integer)
> +RETURNS text[]
> +LANGUAGE sql STABLE
> +AS $_$
> +SELECT array_agg(tag)
> +FROM (
> +SELECT BugTag.tag FROM BugTag WHERE BugTag.bug = $1
> +UNION ALL
> +SELECT NULL::text
> +) AS tag;
> +$_$;
> +
> +COMMENT ON FUNCTION bugsummary_tag_names IS

TIL that this is valid syntax as of PostgreSQL 10:

"Allow the specification of a function name without arguments in DDL commands, 
if it is unique (Peter Eisentraut)"

> +'Return an array of the tag names on the given bug, plus NULL; this is 
> suitable for constructing BugSummaryJournal rows when handling changes to 
> BugTaskFlat.';
> +
> +-- Modify the existing bugsummary_locations to accept an array of tags
> +-- rather than looking them up for itself.
> +DROP FUNCTION bugsummary_locations;
> +CREATE FUNCTION bugsummary_locations(btf_row bugtaskflat, tags text[])
> +RETURNS SETOF bugsummary
> +LANGUAGE plpgsql
> +AS $$
> +BEGIN
> +IF btf_row.duplicateof IS NOT NULL THEN
> +RETURN;
> +END IF;
> +RETURN QUERY
> +SELECT
> +CAST(NULL AS integer) AS id,
> +CAST(1 AS integer) AS count,
> +bug_targets.product, bug_targets.productseries,
> +bug_targets.distribution, bug_targets.distroseries,
> +bug_targets.sourcepackagename,
> +bug_viewers.viewed_by, bug_tags.tag, btf_row.status,
> +btf_row.milestone, btf_row.importance,
> +btf_row.latest_patch_uploaded IS NOT NULL AS has_patch,
> +bug_viewers.access_policy
> +FROM
> +bugsummary_targets(btf_row) AS bug_targets,
> +unnest(tags) AS bug_tags (tag),
> +bugsummary_viewers(btf_row) AS bug_viewers;
> +END;
> +$$;
> +
> +-- Modify the existing bugsummary_journal_bugtaskflat to accept an array of
> +-- tags, and to return a set of rows to the caller rather than inserting
> +-- them into a temporary table.  This can now be just SQL rather than
> +-- PL/pgSQL.
> +DROP FUNCTION bugsummary_journal_bugtaskflat;
> +CREATE FUNCTION bugsummary_journal_bugtaskflat(btf_row bugtaskflat, tags 
> text[], _count integer)
> +RETURNS SETOF bugsummaryjournal
> +LANGUAGE sql
> +AS $$
> +SELECT
> +CAST(NULL AS integer) as id,
> +_count, product, productseries, distribution,
> +distroseries, sourcepackagename, viewed_by, tag,
> +status, milestone, importance, has_patch, access_policy
> +FROM bugsummary_locations(btf_row, tags);
> +$$;
> +
> +-- Rewrite the existing bugtaskflat_maintain_bug_summary as a
> +-- statement-level trigger.
> +CREATE OR REPLACE FUNCTION bugtaskflat_maintain_bug_summary()
> +RETURNS trigger
> +LANGUAGE plpgsql SECURITY DEFINER
> +SET search_path TO 'public'
> +AS $$
> +DECLARE
> +summary_queries text[] DEFAULT '{}';
> +BEGIN
> +-- Work out the subqueries we need to compute the set of
> +-- BugSummaryJournal rows that should be inserted.  (We have to use
> +-- dynamic commands for this, because the transition tables are not
> +-- visible in functions called by this trigger function.)
> +IF TG_OP = 'DELETE' OR TG_OP = 'UPDATE' THEN
> +summary_queries := array_append(summary_queries, $_$
> +SELECT summary.*
> +FROM
> +

[Launchpad-reviewers] [Merge] ~wgrant/launchpad:optimise-gina into launchpad:master

[William Grant]

William Grant has proposed merging ~wgrant/launchpad:optimise-gina into 
launchpad:master.

Commit message:
Optimise gina source and binary DB lookups

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/+git/launchpad/+merge/377080

The relatively modern SourcePackagePublishingHistory.sourcepackagename
and BinaryPackagePublishingHistory.binarypackagename denormalised fields
can improve gina's plans by orders of magnitude in some cases.

This also fixes gina's binary handlers (not used on production since
2006) to correctly deal with multiple archives per distribution (it's
been incorrect since 2007's ArchiveRework).
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
~wgrant/launchpad:optimise-gina into launchpad:master.
diff --git a/database/schema/patch-2210-01-3.sql b/database/schema/patch-2210-01-3.sql
new file mode 100644
index 000..483ad39
--- /dev/null
+++ b/database/schema/patch-2210-01-3.sql
@@ -0,0 +1,7 @@
+-- Copyright 2019 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+
+CREATE INDEX binarypackagepublishinghistory__archive__bpr__idx
+ON binarypackagepublishinghistory (archive, binarypackagerelease);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 01, 3);
diff --git a/lib/lp/soyuz/scripts/gina/handlers.py b/lib/lp/soyuz/scripts/gina/handlers.py
index 7d167d5..66d2aeb 100644
--- a/lib/lp/soyuz/scripts/gina/handlers.py
+++ b/lib/lp/soyuz/scripts/gina/handlers.py
@@ -560,9 +560,11 @@ class SourcePackageHandler:
 SourcePackagePublishingHistory.distroseries =
 DistroSeries.id AND
 SourcePackagePublishingHistory.archive = %s AND
+SourcePackagePublishingHistory.sourcepackagename = %s AND
 DistroSeries.distribution = %s
 """ % sqlvalues(sourcepackagename, version,
 distroseries.main_archive,
+sourcepackagename,
 distroseries.distribution)
 ret = SourcePackageRelease.select(query,
 clauseTables=['SourcePackagePublishingHistory', 'DistroSeries'],
@@ -740,7 +742,8 @@ class BinaryPackageHandler:
 architecture = binarypackagedata.architecture
 
 clauseTables = ["BinaryPackageRelease", "DistroSeries",
-"BinaryPackageBuild", "DistroArchSeries"]
+"DistroArchSeries", "BinaryPackageBuild",
+"BinaryPackagePublishingHistory"]
 distroseries = distroarchseries.distroseries
 
 # When looking for binaries, we need to remember that they are
@@ -748,13 +751,18 @@ class BinaryPackageHandler:
 # distribution and the architecture tag of the distroarchseries
 # they were built for
 query = (
+"BinaryPackagePublishingHistory.archive = %s AND "
+"BinaryPackagePublishingHistory.binarypackagename = %s AND "
+"BinaryPackageRelease.id ="
+" BinaryPackagePublishingHistory.binarypackagerelease AND "
 "BinaryPackageRelease.binarypackagename=%s AND "
 "BinaryPackageRelease.version=%s AND "
 "BinaryPackageRelease.build = BinaryPackageBuild.id AND "
 "BinaryPackageBuild.distro_arch_series = DistroArchSeries.id AND "
 "DistroArchSeries.distroseries = DistroSeries.id AND "
-"DistroSeries.distribution = %d" %
-(binaryname.id, quote(version), distroseries.distribution.id))
+"DistroSeries.distribution = %s" %
+sqlvalues(distroseries.main_archive, binaryname, binaryname,
+  version, distroseries.distribution))
 
 if architecture != "all":
 query += ("AND DistroArchSeries.architecturetag = %s" %
@@ -762,7 +770,7 @@ class BinaryPackageHandler:
 
 try:
 bpr = BinaryPackageRelease.selectOne(
-query, clauseTables=clauseTables)
+query, clauseTables=clauseTables, distinct=True)
 except SQLObjectMoreThanOneResultError:
 # XXX kiko 2005-10-27: Untested
 raise MultiplePackageReleaseError("Found more than one "
@@ -845,9 +853,10 @@ class BinaryPackageHandler:
 query = ("BinaryPackageBuild.source_package_release = %d AND "
  "BinaryPackageBuild.distro_arch_series = "
  "DistroArchSeries.id AND "
+ "BinaryPackageBuild.archive = %d AND "
  "DistroArchSerie

Re: [Launchpad-reviewers] [Merge] ~cjwatson/launchpad:db-oci-recipe-target into launchpad:db-devel

[William Grant]

Diff comments:

> diff --git a/database/schema/patch-2210-08-0.sql 
> b/database/schema/patch-2210-08-0.sql
> new file mode 100644
> index 000..dd3da72
> --- /dev/null
> +++ b/database/schema/patch-2210-08-0.sql
> @@ -0,0 +1,60 @@
> +-- Copyright 2019 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +CREATE TABLE OCIRecipeName (
> +id serial PRIMARY KEY,
> +name text NOT NULL,

This presumably wants to be unique.

> +CONSTRAINT valid_name CHECK (valid_name(name))
> +);
> +
> +COMMENT ON TABLE OCIRecipeName IS 'A name of an Open Container Initiative 
> recipe.';
> +COMMENT ON COLUMN OCIRecipeName.name IS 'A lowercase name identifying an OCI 
> recipe.';
> +
> +CREATE INDEX ocirecipename__name__trgm ON OCIRecipeName
> +USING gin (name trgm.gin_trgm_ops);
> +
> +CREATE TABLE OCIRecipeTarget (

For something that's going to be referenced a lot by e.g. the bug API, this is 
an awful name. Sadly I can't think of anything better, but I feel like we're 
going to regret this even more than GitRepository vs. GitRepo.

> +id serial PRIMARY KEY,
> +date_created timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT 
> TIME ZONE 'UTC') NOT NULL,
> +date_last_modified timestamp without time zone DEFAULT 
> (CURRENT_TIMESTAMP AT TIME ZONE 'UTC') NOT NULL,
> +registrant integer NOT NULL REFERENCES person,
> +distribution integer NOT NULL REFERENCES distribution,
> +ocirecipename integer NOT NULL REFERENCES ocirecipename,
> +description text,
> +bug_reporting_guidelines text,
> +bug_reported_acknowledgement text,
> +enable_bugfiling_duplicate_search boolean DEFAULT true NOT NULL
> +);
> +
> +COMMENT ON TABLE OCIRecipeTarget IS 'A target (pillar and name) for Open 
> Container Initiative recipes.';
> +COMMENT ON COLUMN OCIRecipeTarget.date_created IS 'The date on which this 
> target was created in Launchpad.';
> +COMMENT ON COLUMN OCIRecipeTarget.date_last_modified IS 'The date on which 
> this target was last modified in Launchpad.';
> +COMMENT ON COLUMN OCIRecipeTarget.registrant IS 'The user who registered 
> this target.';
> +COMMENT ON COLUMN OCIRecipeTarget.distribution IS 'The distribution that 
> this target belongs to.';
> +COMMENT ON COLUMN OCIRecipeTarget.ocirecipename IS 'The name of this 
> target.';
> +COMMENT ON COLUMN OCIRecipeTarget.description IS 'A short description of 
> this target.';
> +COMMENT ON COLUMN OCIRecipeTarget.bug_reporting_guidelines IS 'Guidelines to 
> the end user for reporting bugs on this target';
> +COMMENT ON COLUMN OCIRecipeTarget.bug_reported_acknowledgement IS 'A message 
> of acknowledgement to display to a bug reporter after they''ve reported a new 
> bug.';
> +COMMENT ON COLUMN OCIRecipeTarget.enable_bugfiling_duplicate_search IS 
> 'Enable/disable a search for possible duplicates when a bug is filed.';
> +
> +CREATE UNIQUE INDEX ocirecipetarget__distribution__ocirecipename__key
> +ON OCIRecipeTarget (distribution, ocirecipename)
> +WHERE distribution IS NOT NULL;
> +
> +CREATE TABLE OCIRecipeTargetSeries (
> +id serial PRIMARY KEY,
> +ocirecipetarget integer NOT NULL REFERENCES ocirecipetarget,
> +name text NOT NULL,
> +CONSTRAINT valid_name CHECK (valid_name(name))
> +);

Do we want status, date_created, registrant?

> +
> +COMMENT ON TABLE OCIRecipeTargetSeries IS 'A series of an Open Container 
> Initiative recipe target, used to allow tracking bugs against multiple 
> versions of images.';
> +COMMENT ON COLUMN OCIRecipeTargetSeries.ocirecipetarget IS 'The target that 
> this series belongs to.';
> +COMMENT ON COLUMN OCIRecipeTargetSeries.name IS 'The name of this series.';
> +
> +CREATE UNIQUE INDEX ocirecipetargetseries__ocirecipetarget__name__key
> +ON OCIRecipeTargetSeries (ocirecipetarget, name);
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 08, 0);


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/373863
Your team Launchpad code reviewers is requested to review the proposed merge of 
~cjwatson/launchpad:db-oci-recipe-target into launchpad:db-devel.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/git-export-issue-access-token into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-export-issue-access-token/+merge/366057
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/product-release-finder-ftp into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/product-release-finder-ftp/+merge/372161
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/git-honour-access-tokens into lp:launchpad

[William Grant]

Review: Approve code

In _verifyAuthParams's "User authentication with no macaroon.  We do no 
additional checks here." case it's very unclear that some other kind of 
authentication has already taken place, and verifyAuthParams isn't in fact the 
thing that's verifying the auth. Otherwise I think this is all safe enough -- 
thanks.
-- 
https://code.launchpad.net/~cjwatson/launchpad/git-honour-access-tokens/+merge/366053
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/das-filter-webservice into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/das-filter-webservice/+merge/372265
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/das-filter-initialize-ds into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/das-filter-initialize-ds/+merge/372264
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/das-filter-model into lp:launchpad

[William Grant]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/soyuz/interfaces/distroarchseries.py'
> --- lib/lp/soyuz/interfaces/distroarchseries.py   2019-07-30 11:38:18 
> +
> +++ lib/lp/soyuz/interfaces/distroarchseries.py   2019-09-04 14:01:22 
> +
> @@ -216,6 +230,21 @@
>  this distro arch series.
>  """
>  
> +def getFilter():
> +"""Get the filter for packages to build for this architecture, if 
> any.
> +
> +Packages are normally built for all available architectures, subject
> +to any constraints in their `Architecture` field.  If a filter is
> +set, then it applies the additional constraint that packages not
> +included by the filter will not be built for this architecture.
> +"""

getSourceFilter, maybe?

> +
> +def isSourceIncluded(sourcepackagerelease):
> +"""Is this source package included in this distro arch series?
> +
> +:param sourcepackagerelease: An `ISourcePackageRelease` to check.
> +"""

I'm not 100% convinced that this should take an SPR rather than an SPN, but it 
probably doesn't matter too much.

> +
>  
>  class IDistroArchSeriesModerate(Interface):
>  


-- 
https://code.launchpad.net/~cjwatson/launchpad/das-filter-model/+merge/372261
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/das-filter-honour into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/das-filter-honour/+merge/372263
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/packageset-is-source-included into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/packageset-is-source-included/+merge/372259
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/generate-key-pair-script into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/generate-key-pair-script/+merge/371732
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/handle-more-gpg-exceptions into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/handle-more-gpg-exceptions/+merge/371763
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/rabbitfixture-0.4.2 into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/rabbitfixture-0.4.2/+merge/371738
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/optimise-commented-bugs-search-more into lp:launchpad

[William Grant]

Review: Approve code

It might end up being more efficient to get the bug IDs from BugTask itself 
instead, but OTOH BugTaskFlat may be hotter in aggregate.
-- 
https://code.launchpad.net/~cjwatson/launchpad/optimise-commented-bugs-search-more/+merge/371750
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/multiple-override-race into lp:launchpad

[William Grant]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/archivepublisher/domination.py'
> --- lib/lp/archivepublisher/domination.py 2014-10-31 10:34:51 +
> +++ lib/lp/archivepublisher/domination.py 2019-08-29 02:11:49 +
> @@ -615,6 +629,29 @@
>  # published, architecture-independent publications; anything
>  # else will have completed domination in the first pass.
>  packages_w_arch_indep = set()
> +supersede = []
> +keep = set()
> +delete = []
> +
> +def plan(pubs, live_versions):
> +cur_supersede, cur_keep, cur_delete = self.planPackageDomination(
> +pubs, live_versions, generalization)
> +supersede.extend(cur_supersede)
> +keep.update(cur_keep)
> +delete.extend(cur_delete)
> +
> +def execute_plan():
> +for pub, dominant in supersede:
> +pub.supersede(dominant, logger=self.logger)
> +# If this is architecture-independent, all publications with
> +# the same context and overrides should be dominated
> +# simultaneously.

"[...], unless one of the plans decided to keep it. For this reason, an 
architecture's plan can't be executed until all architectures have been 
planned."?

> +if not pub.architecture_specific:
> +for dominated in pub.getOtherPublications():
> +if dominated != pub and dominated not in keep:
> +dominated.supersede(dominant, logger=self.logger)
> +for pub in delete:
> +pub.requestDeletion(None)

I'd suggest logging before the two loops, since they can potentially perform a 
lot of slowish updates, or even deadlock.

>  
>  for distroarchseries in distroseries.architectures:
>  self.logger.info(


-- 
https://code.launchpad.net/~cjwatson/launchpad/multiple-override-race/+merge/368023
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/empty-build-artifacts into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/empty-build-artifacts/+merge/371975
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/chase-email-alias into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/chase-email-alias/+merge/371932
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

[Launchpad-reviewers] [Merge] lp:~wgrant/launchpad/dmp-redirect-2 into lp:launchpad

[William Grant]

William Grant has proposed merging lp:~wgrant/launchpad/dmp-redirect-2 into 
lp:launchpad.

Commit message:
Also allow archive mirrors to redirect pool files.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1836712 in Launchpad itself: "Launchpad mirror prober considers http 
redirects broken"
  https://bugs.launchpad.net/launchpad/+bug/1836712

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/dmp-redirect-2/+merge/371705

The base ProberProtocol is now unused, but that refactoring can wait.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of 
lp:~wgrant/launchpad/dmp-redirect-2 into lp:launchpad.
=== modified file 'lib/lp/registry/doc/distribution-mirror.txt'
--- lib/lp/registry/doc/distribution-mirror.txt	2019-07-23 14:52:01 +
+++ lib/lp/registry/doc/distribution-mirror.txt	2019-08-23 02:32:08 +
@@ -767,6 +767,19 @@
 >>> archive_mirror.getOverallFreshness().title
 'Up to date'
 
+apt has supported HTTP redirects since Ubuntu 9.04, so mirrors that redirect are treated as good:
+
+>>> archive_mirror = mirrorset.getByName('archive-redirect-mirror')
+>>> mirror_arch_series = (
+... archive_mirror.getSummarizedMirroredArchSeries())
+
+# We only have a few publishing records, so most of the cdimage mirrors
+# will have Unknown as their freshness.
+>>> for mirror_arch_series in mirror_arch_series:
+... print (mirror_arch_series.distro_arch_series.displayname,
+...mirror_arch_series.freshness.title)
+(u'Ubuntu Warty i386', 'Up to date')
+
 
 Now we check the MirrorCDImageDistroSeriess of a cdimage mirror.
 

=== modified file 'lib/lp/registry/scripts/distributionmirror_prober.py'
--- lib/lp/registry/scripts/distributionmirror_prober.py	2019-07-23 14:52:01 +
+++ lib/lp/registry/scripts/distributionmirror_prober.py	2019-08-23 02:32:08 +
@@ -505,7 +505,7 @@
 # there.
 arch_or_source_mirror.freshness = MirrorFreshness.UNKNOWN
 for freshness, url in freshness_url_map.items():
-prober = ProberFactory(url)
+prober = RedirectAwareProberFactory(url)
 deferred = request_manager.run(prober.request_host, prober.probe)
 deferred.addCallback(
 self.setMirrorFreshness, arch_or_source_mirror, freshness,

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/person-api-cache-logo-mugshot into lp:launchpad

[William Grant]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py   2019-08-07 00:04:58 +
> +++ lib/lp/registry/model/person.py   2019-08-21 17:10:05 +
> @@ -4088,6 +4096,17 @@
>  column = row[index]
>  index += 1
>  decorator(result, column)
> +if need_icon:
> +icon = row[index]
> +index += 1
> +cache.icon = icon
> +if need_api:
> +logo = row[index]
> +index += 1
> +cache.logo = logo
> +mugshot = row[index]
> +index += 1
> +cache.mugshot = mugshot

Is this hunk necessary? They're FKs, so loading the objects into the Storm 
cache should be sufficient AFAICS.

>  return result
>  return DecoratedResultSet(raw_result,
>  pre_iter_hook=preload_for_people,


-- 
https://code.launchpad.net/~cjwatson/launchpad/person-api-cache-logo-mugshot/+merge/371597
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/git-honour-access-tokens into lp:launchpad

[William Grant]

Review: Needs Fixing code



Diff comments:

> 
> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py 2019-05-09 15:44:59 +
> +++ lib/lp/code/xmlrpc/git.py 2019-05-10 16:51:16 +
> @@ -390,17 +397,18 @@
>  getUtility(IGitRefScanJobSource).create(
>  removeSecurityProxy(repository))
>  
> +@return_fault
>  def authenticateWithPassword(self, username, password):
>  """See `IGitAPI`."""
> -# XXX cjwatson 2016-10-06: We only support free-floating macaroons
> -# at the moment, not ones bound to a user.
> -if not username:
> -verified = self._verifyMacaroon(password)
> -if verified:
> -auth_params = {"macaroon": password}
> -if _is_issuer_internal(verified):
> -auth_params["user"] = LAUNCHPAD_SERVICES
> -return auth_params
> +user = getUtility(IPersonSet).getByName(username) if username else 
> None
> +verified = self._verifyMacaroon(password, user=user)
> +if verified:
> +auth_params = {"macaroon": password}
> +if user is not None:
> +auth_params["uid"] = user.id

Nothing actually enforces that the user was checked as part of the 
verification. The CodeImportJob verifyPrimaryCaveat checks that it isn't there, 
but SnapBuildJob's doesn't. I think we need to ensure the user was verified and 
matches the input somewhere around this level to avoid mistakes.

I also wonder if we want to set an extra auth parameter when the user is 
authenticated by something other than a macaroon, to avoid macaroon being 
subtractive from user/uid.

> +elif _is_issuer_internal(verified):
> +auth_params["user"] = LAUNCHPAD_SERVICES
> +return auth_params
>  # Only macaroons are supported for password authentication.
>  return faults.Unauthorized()
>  


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-honour-access-tokens/+merge/366053
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/multiple-override-race into lp:launchpad

[William Grant]

Review: Needs Fixing code



Diff comments:

> === modified file 'lib/lp/archivepublisher/domination.py'
> --- lib/lp/archivepublisher/domination.py 2014-10-31 10:34:51 +
> +++ lib/lp/archivepublisher/domination.py 2019-05-28 23:48:46 +
> @@ -397,6 +397,17 @@
>  are listed in `live_versions` are marked as Deleted.
>  :param generalization: A `GeneralizedPublication` helper representing
>  the kind of publications these are: source or binary.
> +:param immutable_check: If True, fail if a deletion would modify an
> +immutable suite (e.g. the RELEASE pocket of a CURRENT series).
> +:param superseded: If not None, append (superseded publication,
> +dominant publication) pairs to this list when marking a
> +publication as superseded.  Binary domination uses this to
> +supersede other publications associated with the superseded
> +ones.
> +:param keep: If not None, add publications that have been confirmed
> +as live to this set.  Binary domination uses this to ensure that
> +these live publications are not superseded when superseding
> +associated publications.

Any reason not to have superseded/keep as return values? It'll be slightly 
longer in callsites, but it's really not very obvious that they're output 
parameters. From the perspective of this function they're also both in the 
past: "superseded" and "kept".

I also wonder if at this point it becomes cleaner to have dominatePackage just 
return its assessment of the publications rather than actually act on them 
(planPackageDomination?). Three different places now invoke BPPH.supersede(), 
and all the information needed for the callsite to perform all actions is 
passed up except for deletions.

>  """
>  live_versions = frozenset(live_versions)
>  
> @@ -630,11 +653,19 @@
>  self.logger.debug("Dominating %s" % name)
>  assert len(pubs) > 0, "Dominating zero binaries!"
>  live_versions = find_live_binary_versions_pass_1(pubs)
> -self.dominatePackage(pubs, live_versions, generalization)
> +self.dominatePackage(
> +pubs, live_versions, generalization,
> +superseded=superseded, keep=keep)
>  if contains_arch_indep(pubs):
>  packages_w_arch_indep.add(name)
>  
> +self.logger.info("Dominating associated binaries...")
> +for pub, dominant in superseded:
> +pub.supersedeAssociated(dominant, keep=keep, logger=self.logger)

"Associated" isn't an existing term in Soyuz AFAIK. It could use a comment to 
explain what's going on here and why we're doing it at such a high level.

And I wonder if supersede(supersede_associated=True) even makes sense any more. 
Encapsulating that logic in the model made sense when it was simple and could 
be encapsulated there, but it can't any more. BPPH._getOtherPublications would 
need to be renamed, cleaned up, and made public, but that's a cleaner interface 
than this weird supersede_associated=True mode which isn't a sensible operation 
at all. _getOtherPublications can probably also lose the override matches, as 
that rule is only there to prevent the latest publication from accidentally 
superseding itself, and the whole purpose of this branch is to do that in a 
more robust way.

This becomes particularly clean if we pull even non-associated BPPH.supersede() 
calls out to here. You just loop over superseded, call _getOtherPublications, 
subtract keep, and invoke supersede() on the pub and the surviving others.

> +
>  packages_w_arch_indep = frozenset(packages_w_arch_indep)
> +superseded = []
> +keep = set()
>  
>  # The second pass attempts to supersede arch-all publications of
>  # older versions, from source package releases that no longer
> @@ -655,7 +686,13 @@
>  assert len(pubs) > 0, "Dominating zero binaries in 2nd pass!"
>  live_versions = find_live_binary_versions_pass_2(
>  pubs, reprieve_cache)
> -self.dominatePackage(pubs, live_versions, generalization)
> +self.dominatePackage(
> +pubs, live_versions, generalization,
> +superseded=superseded, keep=keep)
> +
> +self.logger.info("Dominating associated binaries...(2nd pass)")
> +for pub, dominant in superseded:
> +pub.supersedeAssociated(dominant, keep=keep, logger=self.logger)

This seems like a pretty acceptable time to factor out the common bits of the 
two passes.

Though, in fact, can't the first pass never superseded an arch-indep pub? It 
finds sources that have them to seed the second pass, but 
find_live_binary_versions_pass_1 only includes architecture-specific.

>  
>  def 

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/livefs-minimal-privacy into lp:launchpad

[William Grant]

Review: Approve code

This isn't quite accurate, since the files' privacy doesn't follow the owner's, 
but that's rare enough that it probably doesn't matter.
-- 
https://code.launchpad.net/~cjwatson/launchpad/livefs-minimal-privacy/+merge/370759
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/snap-store-secrets-encrypt into lp:launchpad

[William Grant]

Review: Approve code



Diff comments:

> 
> === added file 'lib/lp/services/crypto/model.py'
> --- lib/lp/services/crypto/model.py   1970-01-01 00:00:00 +
> +++ lib/lp/services/crypto/model.py   2019-06-27 21:46:50 +
> @@ -0,0 +1,122 @@
> +# Copyright 2019 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""A container for data encrypted at rest using configured keys."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> +'NaClEncryptedContainerBase',
> +]
> +
> +import base64
> +
> +from nacl.exceptions import CryptoError as NaClCryptoError
> +from nacl.public import (
> +PrivateKey,
> +PublicKey,
> +SealedBox,
> +)
> +import six
> +from zope.interface import implementer
> +
> +from lp.services.crypto.interfaces import (
> +CryptoError,
> +IEncryptedContainer,
> +)
> +
> +
> +def _make_public_key(public_key_bytes):
> +pass

This seems to be unused.

> +
> +
> +@implementer(IEncryptedContainer)
> +class NaClEncryptedContainerBase:
> +"""A container that can encrypt and decrypt data using NaCl.
> +
> +See `IEncryptedContainer`.
> +"""
> +
> +@property
> +def public_key_bytes(self):
> +"""The serialised public key as a byte string.
> +
> +Concrete implementations must provide this.
> +"""
> +raise NotImplementedError
> +
> +@property
> +def public_key(self):
> +"""The public key as a L{nacl.public.PublicKey}."""
> +if self.public_key_bytes is not None:
> +try:
> +return PublicKey(self.public_key_bytes)
> +except NaClCryptoError as e:
> +six.raise_from(CryptoError(str(e)), e)
> +else:
> +return None
> +
> +@property
> +def can_encrypt(self):
> +try:
> +return self.public_key is not None
> +except CryptoError:
> +return False
> +
> +def encrypt(self, data):
> +"""See `IEncryptedContainer`."""
> +if self.public_key is None:
> +raise RuntimeError("No public key configured")
> +try:
> +data_encrypted = SealedBox(self.public_key).encrypt(data)
> +except NaClCryptoError as e:
> +six.raise_from(CryptoError(str(e)), e)
> +return (
> +base64.b64encode(self.public_key_bytes).decode("UTF-8"),
> +base64.b64encode(data_encrypted).decode("UTF-8"))
> +
> +@property
> +def private_key_bytes(self):
> +"""The serialised private key as a byte string.
> +
> +Concrete implementations must provide this.
> +"""
> +raise NotImplementedError
> +
> +@property
> +def private_key(self):
> +"""The private key as a L{nacl.public.PrivateKey}."""
> +if self.private_key_bytes is not None:
> +try:
> +return PrivateKey(self.private_key_bytes)
> +except NaClCryptoError as e:
> +six.raise_from(CryptoError(str(e)), e)
> +else:
> +return None
> +
> +@property
> +def can_decrypt(self):
> +try:
> +return self.private_key is not None
> +except CryptoError:
> +return False
> +
> +def decrypt(self, data):
> +"""See `IEncryptedContainer`."""
> +public_key, encrypted = data
> +try:
> +public_key_bytes = base64.b64decode(public_key.encode("UTF-8"))
> +encrypted_bytes = base64.b64decode(encrypted.encode("UTF-8"))
> +except TypeError as e:
> +six.raise_from(CryptoError(str(e)), e)
> +if public_key_bytes != self.public_key_bytes:
> +raise ValueError(
> +"Public key %r does not match configured public key %r" %
> +(public_key_bytes, self.public_key_bytes))
> +if self.private_key is None:
> +raise ValueError("No private key configured")
> +try:
> +return SealedBox(self.private_key).decrypt(encrypted_bytes)
> +except NaClCryptoError as e:
> +six.raise_from(CryptoError(str(e)), e)
> 
> === modified file 'lib/lp/snappy/model/snapstoreclient.py'
> --- lib/lp/snappy/model/snapstoreclient.py2019-06-20 16:33:19 +
> +++ lib/lp/snappy/model/snapstoreclient.py2019-06-27 21:46:50 +
> @@ -320,7 +349,15 @@
>  raise BadRefreshResponse(response.text)
>  # Set a new dict here to avoid problems with security proxies.
>  new_secrets = dict(snap.store_secrets)
> -new_secrets["discharge"] = response_data["discharge_macaroon"]
> +container = getUtility(IEncryptedContainer, "snap-store-secrets")
> +if container.can_encrypt:
> +new_secrets["discharge_encrypted"] = (
> +

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/meta-lp-deps/geoip2 into lp:meta-lp-deps

[William Grant]

Review: Approve


-- 
https://code.launchpad.net/~cjwatson/meta-lp-deps/geoip2/+merge/371092
Your team Launchpad code reviewers is subscribed to branch lp:meta-lp-deps.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/geoip2 into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/geoip2/+merge/371095
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/geoip-country into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/geoip-country/+merge/371044
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/test-apachelogparser-ipv6 into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/test-apachelogparser-ipv6/+merge/370769
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/snap-find-by-store-name into lp:launchpad

[William Grant]

Review: Approve code db



Diff comments:

> 
> === modified file 'lib/lp/snappy/tests/test_snap.py'
> --- lib/lp/snappy/tests/test_snap.py  2019-07-15 17:15:25 +
> +++ lib/lp/snappy/tests/test_snap.py  2019-07-24 12:45:34 +
> @@ -1674,6 +1674,24 @@
>  [snaps[0], snaps[2], snaps[4], snaps[6]],
>  getUtility(ISnapSet).findByURLPrefixes(prefixes, 
> owner=owners[0]))
>  
> +def test_findByStoreName(self):
> +# ISnapSet.findByStoreName returns visible Snaps with the given
> +# store name.
> +store_names = ["foo", "bar"]
> +owners = [self.factory.makePerson() for i in range(2)]
> +snaps = []
> +for store_name in store_names:
> +for owner in owners:
> +snaps.append(self.factory.makeSnap(
> +registrant=owner, owner=owner, store_name=store_name))
> +snaps.append(self.factory.makeSnap())
> +self.assertContentEqual(
> +snaps[:2], getUtility(ISnapSet).findByStoreName(store_names[0]))
> +self.assertContentEqual(
> +[snaps[0]],
> +getUtility(ISnapSet).findByStoreName(
> +store_names[0], owner=owners[0]))

Can you add a visible_by_user test too?

> +
>  def test_getSnapcraftYaml_bzr_snap_snapcraft_yaml(self):
>  def getInventory(unique_name, dirname, *args, **kwargs):
>  if dirname == "snap":
> @@ -3004,6 +3022,78 @@
>  [ws_snaps[i] for i in (0, 1, 4, 5, 8, 9, 12, 13)],
>  [entry["self_link"] for entry in response.jsonBody()["entries"]])
>  
> +def test_findByStoreName(self):
> +# lp.snaps.findByStoreName returns visible Snaps with the given
> +# store name.
> +persons = [self.factory.makePerson(), self.factory.makePerson()]
> +store_names = ["foo", "bar"]
> +snaps = []
> +for store_name in store_names:
> +for person in persons:
> +for private in (False, True):
> +snaps.append(self.factory.makeSnap(
> +registrant=person, owner=person, private=private,
> +store_name=store_name))
> +with admin_logged_in():
> +person_urls = [api_url(person) for person in persons]
> +ws_snaps = [
> +self.webservice.getAbsoluteUrl(api_url(snap))
> +for snap in snaps]
> +commercial_admin = (
> +getUtility(ILaunchpadCelebrities).commercial_admin.teamowner)
> +logout()
> +# Anonymous requests can only see public snaps.
> +anon_webservice = LaunchpadWebServiceCaller("test", "")
> +response = anon_webservice.named_get(
> +"/+snaps", "findByStoreName", store_name=store_names[0],
> +api_version="devel")
> +self.assertEqual(200, response.status)
> +self.assertContentEqual(
> +[ws_snaps[0], ws_snaps[2]],
> +[entry["self_link"] for entry in response.jsonBody()["entries"]])
> +response = anon_webservice.named_get(
> +"/+snaps", "findByStoreName", store_name=store_names[0],
> +owner=person_urls[0], api_version="devel")
> +self.assertEqual(200, response.status)
> +self.assertContentEqual(
> +[ws_snaps[0]],
> +[entry["self_link"] for entry in response.jsonBody()["entries"]])
> +# persons[0] can see both public snaps with this store name, as well
> +# as their own private snap.
> +webservice = webservice_for_person(
> +persons[0], permission=OAuthPermission.READ_PRIVATE)
> +response = webservice.named_get(
> +"/+snaps", "findByStoreName", store_name=store_names[0],
> +api_version="devel")
> +self.assertEqual(200, response.status)
> +self.assertContentEqual(
> +ws_snaps[:3],
> +[entry["self_link"] for entry in response.jsonBody()["entries"]])
> +response = webservice.named_get(
> +"/+snaps", "findByStoreName", store_name=store_names[0],
> +owner=person_urls[0], api_version="devel")
> +self.assertEqual(200, response.status)
> +self.assertContentEqual(
> +ws_snaps[:2],
> +[entry["self_link"] for entry in response.jsonBody()["entries"]])
> +# Admins can see all snaps with this store name.
> +commercial_admin_webservice = webservice_for_person(
> +commercial_admin, permission=OAuthPermission.READ_PRIVATE)
> +response = commercial_admin_webservice.named_get(
> +"/+snaps", "findByStoreName", store_name=store_names[0],
> +api_version="devel")
> +self.assertEqual(200, response.status)
> +self.assertContentEqual(
> +ws_snaps[:4],
> +[entry["self_link"] for entry in response.jsonBody()["entries"]])
> +response = 

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/geoip-ip-address-handling into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/geoip-ip-address-handling/+merge/369729
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/close-account-deleted-ppa into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/close-account-deleted-ppa/+merge/371119
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/team-members-ordering into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/team-members-ordering/+merge/371035
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/librarian-swift-assertion-errors into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/librarian-swift-assertion-errors/+merge/370960
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/gina-bullseye into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/gina-bullseye/+merge/370298
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/use-bzr-policy-open into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/use-bzr-policy-open/+merge/368761
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/snap-base-none into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-base-none/+merge/369251
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/optimise-commented-bugs-search into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/optimise-commented-bugs-search/+merge/369656
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/more-robust-debdiff-timeout into lp:launchpad

[William Grant]

Review: Approve code


-- 
https://code.launchpad.net/~cjwatson/launchpad/more-robust-debdiff-timeout/+merge/369535
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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

Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/improve-bug-commenter-search into lp:launchpad

[William Grant]

Review: Approve code

I'd like to see a query plan from a couple of representative users, but it was 
already pretty bad and timed out for heavy users I suppose.
-- 
https://code.launchpad.net/~cjwatson/launchpad/improve-bug-commenter-search/+merge/369526
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

___
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