Thanks for the feedback! I have applied most suggestions, and now I am ready to
implement the missing test cases.
Diff comments:
> diff --git a/lib/lp/code/model/cibuild.py b/lib/lp/code/model/cibuild.py
> index f9c9031..e5893b3 100644
> --- a/lib/lp/code/model/cibuild.py
> +++ b/lib/lp/code/model/cibuild.py
> @@ -329,6 +337,136 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
> store.flush()
> return cibuild
>
> + def findByGitRepository(self, git_repository, commit_sha1s=None):
> + """See `ICIBuildSet`."""
> + clauses = [CIBuild.git_repository == git_repository]
> + if commit_sha1s is not None:
> + clauses.append(CIBuild.commit_sha1.is_in(commit_sha1s))
> + return IStore(CIBuild).find(CIBuild, *clauses)
> +
> + def _isBuildableArchitectureAllowed(self, das):
> + """Check whether we may build for a buildable `DistroArchSeries`.
> +
> + The caller is assumed to have already checked that a suitable chroot
> + is available (either directly or via
> + `DistroSeries.buildable_architectures`).
> + """
> + return (
> + das.enabled
> + # We only support builds on virtualized builders at the moment.
> + and das.processor.supports_virtualized)
> +
> + def _isArchitectureAllowed(self, das, pocket, snap_base=None):
> + return (
> + das.getChroot(pocket=pocket) is not None
> + and self._isBuildableArchitectureAllowed(das))
> +
> + def requestBuild(self, git_repository, commit_sha1, distro_arch_series):
> + """See `ICIBuildSet`."""
> + pocket = PackagePublishingPocket.UPDATES
> + if not self._isArchitectureAllowed(distro_arch_series, pocket):
> + raise CIBuildDisallowedArchitecture(distro_arch_series, pocket)
> +
> + result = IStore(CIBuild).find(
> + CIBuild,
> + CIBuild.git_repository == git_repository,
> + CIBuild.commit_sha1 == commit_sha1,
> + CIBuild.distro_arch_series == distro_arch_series)
> + if not result.is_empty():
> + raise CIBuildAlreadyRequested
> +
> + build = self.new(git_repository, commit_sha1, distro_arch_series)
> + build.queueBuild()
> + notify(ObjectCreatedEvent(build))
> + return build
> +
> + def _determineDASesToBuild(self, configuration, logger=None):
Moved it to a module level function.
When it comes to `@staticmethod` I go with Guido v.R.:
> Honestly, staticmethod was something of a mistake -- I was trying to
do something like Java class methods but once it was released I found
what was really needed was classmethod. But it was too late to get rid
of staticmethod.
> + """Generate distroarchseries to build for this configuration."""
> + architectures_by_series = {}
> + for stage in configuration.pipeline:
> + for job_name in stage:
> + if job_name not in configuration.jobs:
> + if logger is not None:
> + logger.error("No job definition for %r", job_name)
> + continue
> + for job in configuration.jobs[job_name]:
> + for architecture in job["architectures"]:
> + architectures_by_series.setdefault(
> + job["series"], set()).add(architecture)
> + # XXX cjwatson 2022-01-21: We have to hardcode Ubuntu for now, since
> + # the .launchpad.yaml format doesn't currently support other
> + # distributions (although nor does the Launchpad build farm).
> + distribution = getUtility(ILaunchpadCelebrities).ubuntu
> + for series_name, architecture_names in
> architectures_by_series.items():
> + try:
> + series = distribution[series_name]
> + except NotFoundError:
> + if logger is not None:
> + logger.error("Unknown Ubuntu series name %s" %
> series_name)
> + continue
> + architectures = {
> + das.architecturetag: das
> + for das in series.buildable_architectures}
> + for architecture_name in architecture_names:
> + try:
> + das = architectures[architecture_name]
> + except KeyError:
> + if logger is not None:
> + logger.error(
> + "%s is not a buildable architecture name in "
> + "Ubuntu %s" % (architecture_name, series_name))
> + continue
> + yield das
> +
> + def requestBuildsForRefs(self, git_repository, ref_paths, logger=None):
> + """See `ICIBuildSet`."""
> + # XXX jugmac00 2022-03-01:
> + # - extract function
> + # - name: get_all_commits_for_paths(repository, paths)
Sounds good, had similar ideas.
> + commit_sha1s = [
> + ref.commit_sha1
> + for ref in GitRef.findByReposAndPaths(
> + [(git_repository, ref_path)
> + for ref_path in ref_paths]).values()]
> + # getCommits performs a web request!
> + commits = getUtility(IGitHostingClient).getCommits(
> + git_repository.getInternalPath(), commit_sha1s,
> + # XXX cjwatson 2022-01-19: We should also fetch
> + # debian/.launchpad.yaml (or perhaps make the path a property of
> + # the repository) once lpcraft and launchpad-buildd support
> + # using alternative paths for builds.
> + filter_paths=[".launchpad.yaml"])
> + for commit in commits:
> + try:
> + configuration = parse_configuration(
> + git_repository, commit["blobs"][".launchpad.yaml"])
> + except CannotParseConfiguration as e:
> + if logger is not None:
> + logger.error(
> + "Cannot parse .launchpad.yaml from %s:%s: %s",
> + git_repository.unique_name, commit["sha1"], e)
> + continue
I tried some refactorings, but it did not become clearer or more readable.
Let's keep it for now.
> + # XXX jugmac00 2022-03-01:
> + # - extract function
> + # - name: try_to_schedule_builds(commit, configuration, logger)
> + for das in self._determineDASesToBuild(configuration):
> + try:
> + if logger is not None:
> + logger.info(
> + "Requesting CI build for %s on %s/%s",
> + commit["sha1"], das.distroseries.name,
> + das.architecturetag)
> + self.requestBuild(git_repository, commit["sha1"], das)
> + except CIBuildAlreadyRequested:
> + pass
> + except Exception as e:
> + if logger is not None:
> + logger.error(
> + "Failed to request CI build for %s on %s/%s: "
> + "%s",
> + commit["sha1"], das.distroseries.name,
> + das.architecturetag, e)
> +
> def getByID(self, build_id):
> """See `ISpecificBuildFarmJobSource`."""
> store = IMasterStore(CIBuild)
> diff --git a/lib/lp/code/model/tests/test_cibuild.py
> b/lib/lp/code/model/tests/test_cibuild.py
> index 133bed2..fbd802b 100644
> --- a/lib/lp/code/model/tests/test_cibuild.py
> +++ b/lib/lp/code/model/tests/test_cibuild.py
> @@ -336,6 +354,211 @@ class TestCIBuildSet(TestCaseWithFactory):
> self.assertContentEqual(
> builds[2:], ci_build_set.findByGitRepository(repositories[1]))
>
> + def makeBuildableDistroArchSeries(self, architecturetag=None,
> + processor=None,
> + supports_virtualized=True,
> + supports_nonvirtualized=True,
> **kwargs):
> + if architecturetag is None:
> + architecturetag = self.factory.getUniqueUnicode("arch")
> + if processor is None:
> + try:
> + processor = getUtility(IProcessorSet).getByName(
> + architecturetag)
> + except ProcessorNotFound:
> + processor = self.factory.makeProcessor(
> + name=architecturetag,
> + supports_virtualized=supports_virtualized,
> + supports_nonvirtualized=supports_nonvirtualized)
> + das = self.factory.makeDistroArchSeries(
> + architecturetag=architecturetag, processor=processor, **kwargs)
> + # Add both a chroot and a LXD image to test that
> + # getAllowedArchitectures doesn't get confused by multiple
> + # PocketChroot rows for a single DistroArchSeries.
> + fake_chroot = self.factory.makeLibraryFileAlias(
> + filename="fake_chroot.tar.gz", db_only=True)
> + das.addOrUpdateChroot(fake_chroot)
> + fake_lxd = self.factory.makeLibraryFileAlias(
> + filename="fake_lxd.tar.gz", db_only=True)
> + das.addOrUpdateChroot(fake_lxd, image_type=BuildBaseImageType.LXD)
> + return das
> +
> + def test_requestCIBuild(self):
> + # requestBuild creates a new CIBuild.
> + repository = self.factory.makeGitRepository()
> + commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
> + das = self.makeBuildableDistroArchSeries()
> +
> + build = getUtility(ICIBuildSet).requestBuild(
> + repository, commit_sha1, das)
> +
> + self.assertTrue(ICIBuild.providedBy(build))
> + self.assertThat(build, MatchesStructure.byEquality(
> + git_repository=repository,
> + commit_sha1=commit_sha1,
> + distro_arch_series=das,
> + status=BuildStatus.NEEDSBUILD,
> + ))
> + store = Store.of(build)
> + store.flush()
> + build_queue = store.find(
> + BuildQueue,
> + BuildQueue._build_farm_job_id ==
> + removeSecurityProxy(build).build_farm_job_id).one()
> + self.assertProvides(build_queue, IBuildQueue)
> + self.assertTrue(build_queue.virtualized)
> + self.assertEqual(BuildQueueStatus.WAITING, build_queue.status)
> +
> + def test_requestBuild_score(self):
> + # CI builds have an initial queue score of 2600.
> + repository = self.factory.makeGitRepository()
> + commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
> + das = self.makeBuildableDistroArchSeries()
> + build = getUtility(ICIBuildSet).requestBuild(
> + repository, commit_sha1, das)
> + queue_record = build.buildqueue_record
> + queue_record.score()
> + self.assertEqual(2600, queue_record.lastscore)
> +
> + def test_requestBuild_rejects_repeats(self):
> + # requestBuild refuses if an identical build was already requested.
> + repository = self.factory.makeGitRepository()
> + commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
> + distro_series = self.factory.makeDistroSeries()
> + arches = [
> + self.makeBuildableDistroArchSeries(distroseries=distro_series)
> + for _ in range(2)]
> + old_build = getUtility(ICIBuildSet).requestBuild(
> + repository, commit_sha1, arches[0])
> + self.assertRaises(
> + CIBuildAlreadyRequested, getUtility(ICIBuildSet).requestBuild,
> + repository, commit_sha1, arches[0])
> + # We can build for a different distroarchseries.
> + getUtility(ICIBuildSet).requestBuild(
> + repository, commit_sha1, arches[1])
> + # Changing the status of the old build does not allow a new build.
> + old_build.updateStatus(BuildStatus.BUILDING)
> + old_build.updateStatus(BuildStatus.FULLYBUILT)
> + self.assertRaises(
> + CIBuildAlreadyRequested, getUtility(ICIBuildSet).requestBuild,
> + repository, commit_sha1, arches[0])
> +
> + def test_requestBuild_virtualization(self):
> + # New builds are virtualized.
> + repository = self.factory.makeGitRepository()
> + commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
> + distro_series = self.factory.makeDistroSeries()
> + for proc_nonvirt in True, False:
> + das = self.makeBuildableDistroArchSeries(
> + distroseries=distro_series, supports_virtualized=True,
> + supports_nonvirtualized=proc_nonvirt)
> + build = getUtility(ICIBuildSet).requestBuild(
> + repository, commit_sha1, das)
> + self.assertTrue(build.virtualized)
> +
> + def test_requestBuild_nonvirtualized(self):
> + # A non-virtualized processor cannot run a CI build.
> + repository = self.factory.makeGitRepository()
> + commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
> + distro_series = self.factory.makeDistroSeries()
> + das = self.makeBuildableDistroArchSeries(
> + distroseries=distro_series, supports_virtualized=False,
> + supports_nonvirtualized=True)
> + self.assertRaises(
> + CIBuildDisallowedArchitecture,
> + getUtility(ICIBuildSet).requestBuild, repository, commit_sha1,
> das)
> +
> + def test__determineDASesToBuild(self):
When a private method is pretty complex, often it should be extract as a class
- with a then public method to use in the previous context. I guess it depends
:-)
> + ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> + distro_serieses = [
> + self.factory.makeDistroSeries(ubuntu) for _ in range(2)]
> + dases = []
> + for distro_series in distro_serieses:
> + for _ in range(2):
> + dases.append(self.makeBuildableDistroArchSeries(
> + distroseries=distro_series))
> + configuration = load_configuration(dedent("""\
> + pipeline:
> + - [build]
> + - [test]
> + jobs:
> + build:
> + series: {distro_serieses[1].name}
> + architectures:
> + - {dases[2].architecturetag}
> + - {dases[3].architecturetag}
> + test:
> + series: {distro_serieses[1].name}
> + architectures:
> + - {dases[2].architecturetag}
> + """.format(distro_serieses=distro_serieses, dases=dases)))
> + ci_build_set = removeSecurityProxy(getUtility(ICIBuildSet))
> + logger = BufferLogger()
> +
> + dases_to_build = list(
> + ci_build_set._determineDASesToBuild(configuration,
> logger=logger))
> +
> + self.assertContentEqual(dases[2:], dases_to_build)
> + self.assertEqual("", logger.getLogBuffer())
> + # XXX error cases for _determineDASesToBuild
> +
> +
> + def test_requestBuildsForRefs(self):
I have seen this pattern in the test for `determine_DASes_to_build`, and I
would prefer not to follow it.
I had quite a hard time to get a grip on the test - and that is not ideal,
especially as I see tests as a documentation for the requirements and I also
want to quickly figure out what is wrong when the test fails.
e.g. when a test fails in the following line, I possibly need to read a lot of
preceeding code and need to use a debugger to figure out that is going wrong.
```
self.assertContentEqual(dases[2:], dases_to_build)
```
On the other hand, for the following line it is pretty clear:
```
self.assertEqual("focal", build.distro_arch_series.distroseries.name)
```
Apart from readability, I do not like to compare two computed values - they
both could be wrong in a way that the assertion succeeds anyway.
The `black` style formatting is something we have opted into, so this will
stay. I have noticed I have overdone it somewhat, so I'll reformat the method
to make it more terse and I think more readable.
> And I think the "set up git repository + git commit" comment is probably not
> very necessary.
I usually create intermediate comments within functions/methods, which I later
use as a method/function/fixture name when I extract that code part. Also, this
is a helper for me while learning a new domain. I'll remove the comments before
finishing the MP.
Thanks for your input!
> + # set up distroseries and buildable architectures
> + ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> + series = self.factory.makeDistroSeries(
> + distribution=ubuntu,
> + name="focal",
> + )
> + self.makeBuildableDistroArchSeries(
> + distroseries=series,
> + architecturetag="amd64"
> + )
> +
> + # set up git repository + git commit
> + configuration = dedent("""\
> + pipeline:
> + - test
> +
> + jobs:
> + test:
> + series: focal
> + architectures: amd64
> + run: echo hello world >output
> + """).encode()
> + repository = self.factory.makeGitRepository()
> + ref_paths = ['refs/heads/master']
> + self.factory.makeGitRefs(repository, ref_paths)
> + encoded_commit_json = {
> + "sha1": "0",
> + "blobs": {
> + ".launchpad.yaml": configuration
> + },
> + }
> + hosting_fixture = self.useFixture(
> + GitHostingFixture(commits=[encoded_commit_json])
> + )
> +
> + getUtility(ICIBuildSet).requestBuildsForRefs(
> + repository, ref_paths,
> + )
> +
> + self.assertEqual(
> + [
> + (("1", ["972c6d2dc6dd5efdad1377c0d224e03eb8f276f7"]),
Thanks!
Actually, I had a look at the method, but (at 10 pm) I could not figure out
what it returns - I probably missed the docstring, and I could have set a
breakpoint.
> + {"filter_paths": [".launchpad.yaml"]})
> + ],
> + hosting_fixture.getCommits.calls
> + )
> +
> + store = IStore(CIBuild)
> + store.flush()
Thanks for the explanation.
> + build = store.find(CIBuild).one()
Good one! Thanks.
> +
> + # check that a build was created
> + self.assertEqual("0", build.commit_sha1)
> + self.assertEqual("focal", build.distro_arch_series.distroseries.name)
> + self.assertEqual("amd64", build.distro_arch_series.architecturetag)
> +
> def test_deleteByGitRepository(self):
> repositories = [self.factory.makeGitRepository() for _ in range(2)]
> builds = []
--
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/416223
Your team Launchpad code reviewers is requested to review the proposed merge of
~jugmac00/launchpad:create-lpcraft-jobs-on-push 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
