launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

On Wed, Aug 28, 2013 at 05:47:36AM -0000, William Grant wrote:
> 20	+ def _allIndexFiles(self, distroseries):
> 21	+ """Return all index files on disk for a distroseries."""
> 
> This method seems like it should mostly be elsewhere. It's not reasonably practical to factor out the commonalities somehow?

I refactored this method a little bit internally to make it less
repetitive, but I'm not sure what you mean in general.  The Publisher
class knows about paths to the various index files, so it seems to make
sense for a method that returns them all to live there.

Do you just mean that _allIndexFiles should be split up or folded in
somewhere else?  I could have written it inline in
_latestNonEmptySeries, but it was pretty verbose when open-coded, and it
seemed nicer to use a generator than to build up a list.

> 57	+ def createSeriesAliases(self):
> 
> This has the slightly weird behavior that a single missing current suite will cause just that alias to point to the old series, while the other suites point to the new one. This probably can't happen in production, because primary archive indices are always all generated for all suites on series creation, even if empty.

The only alternatives I can think of here are:

 1) this behaviour
 2) sometimes have aliases which are dangling symlinks
 3) hold all the aliases back to the older series
 4) don't create any aliases at all on mismatch

I don't think 2) should be allowed.  3) has the problem that if we're
considering cases that ought not to happen in production then it's
entirely possible that there's no consistent alias to select.  So the
alternatives appear to be 1) or 4).  4) would be confusing in a
different way ("why are there no aliases?") - what do you think?

> 732	distribution = self.distribution
> 733	if to_series is not None:
> 734	result = getUtility(IDistroSeriesSet).queryByName(
> 735	- distribution, to_series)
> 736	+ distribution, to_series, follow_aliases=True)
> 737	if result is None:
> 738	raise NoSuchDistroSeries(to_series)
> 739	series = result
> 
> This is probably fairly confusing for PPAs. You can copy to devel and it'll actually be different from the current devel symlink in the PPA, with no way to tell beforehand. But we probably can't avoid it given the current fairly braindead method signature.

Yes.  As discussed at the releng sprint, I think we do want this
behaviour as it has the right properties of encouraging developers to
stick with the current development series, but it's true that it can be
confusing.

I've added a set of comments to the interfaces, which is probably the
best I can do.

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