Code review comment for lp:~jtv/launchpad/db-bug-786970

Thanks Gavin.

> However, I have a major beef with it: PlainPackageCopyJob is no longer
> a plain copy job; it's now heavily intertwined with derivative
> distributions. I think PlainPackageCopyJob should be restored, and a
> new subclass created, something like DerivativePackageCopyJob, that
> knows about DSDs and so forth. It's unfortunate that that involves a
> lot of boilerplate code.

That seems like a job for a separate branch though; it will affect a lot more than what's in this branch.

Do I understand that correctly? If so, it seems easier to land this branch first and then make a separate card of splitting up the various usages of PPCJ. Would be nice to get the present branch off my hands!

> Other than that, I think it's a shame that packages can only be copied
> one at a time, but I can see this is a far from trivial problem to
> solve. It does mean that there's no longer an all-or-nothing guarantee
> when copying multiple packages. A *big* downside is that users must
> check all DSD comments for errors when they request multiple copies. I
> assume both of those are acceptable compromises for now.

Yes— we considered that, but it turns out that multi-parent breaks that anyway. You could be selecting DSDs from different parent archives, which have to go into separate jobs regardless!

So in the final analysis we can blame this on the post-last-minute change in requirements.

> + try:
> + self.attemptCopy()
> + except CannotCopy, e:
> + self.abort()
> + self.reportFailure(e)
> + self.commit()
>
> Fwiw, the job runner commits after run(), so only the abort() is
> needed.

Nice one! That commit was there from a time when the exception was still propagated, which I preserved in this branch initially but was then rightly asked to change.

> + def findMatchingDSDs(self):
> [...]
> + source_distro = self.source_archive.distribution
> + return [
> + dsd
> + for dsd in candidates
> + if dsd.parent_series.distribution == source_distro]
>
> Can you do with with IDs instead?

Sure. Silly thing that the ORM needs to fetch the distribution there, really.

> + def test_findMatchingDSDs_matches_all_DSDs_for_job(self):
> + # findMatchingDSDs finds matching DSDs for any of the packages
> + # in the job.
> + dsd = self.factory.makeDistroSeriesDifference()
> + job = removeSecurityProxy(self.makeJob(dsd))
>
> This should be naked_job.
>
> Also in test_findMatchingDSDs_ignores_other_source_series.

Fixed.

Jeroen