From 7388890f4f71cce812cbb4de12695ac726a29b1f Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Feb 21 2018 07:21:46 +0000 Subject: Handle tricky candidate compose fedfind situations better Talking to @puiterwijk about the tricky corner cases involving event creation for candidate composes today made me realize I could actually handle it rather better in relvalconsumer and wikitcms together. First off, we don't really need to completely fail event creation if we can't create the download page because we can't find the fedfind release for the event. We can just handle that as a warning and complete the rest of the event creation. This is especially true since we can always go back and create the download page later. The other tweaks are to do with the corner cases with candidate compose syncing. When candidate composes are produced, they appear in the initial compose output location (kojipkgs) first; when the 'compose complete' fedmsg appears, it means the compose is available there. Shortly after this happens, the compose is (or should be...sometimes this fails) imported to PDC. Some time after *that* (the time varies at present, as this is triggered manually, not automatically), the compose is synced to another location, this one in the mirror system (alt/stage). In fedfind terms, the compose in its original, non-mirrored location is a 'Production' instance, while the compose in its synced, mirrored location is a 'Compose'. The validation event version for any candidate compose is based on its label, not its compose ID. This happens to be sufficient information to determine the *synced* location, but not the *unsynced* location. So if all you have is this info, you can instantiate the Compose instance for the compose - and thus find its images *if it has been synced* - but not the Production instance - you can't find the compose's images if it has not yet been synced. To complicate matters a bit, once the compose is imported to PDC it *is* possible to get the Production instance from the label, because fedfind can query PDC to figure out the compose ID from the label. With all that in mind, there are a couple of changes here. At one point in time we had an icky hack to allow relvalconsumer to pass a compose ID right into `ComposeEvent.create()` - that allowed us to create the event for a candidate compose as soon as it was created, before even it was imported to PDC, and get a download page with the non-synced image URLs. Here we sort of re-introduce that, but in a rather cleaner way: we add a private attribute, `ComposeEvent._cid`, which gets populated if (and only if) you use `get_validation_event(cid='somecid')` to get a `ComposeEvent`. The fedfind release properties are tweaked to take advantage of this. If we can't find the `ff_release` from the event version information (which could mean the compose is done but has not yet been synced), and we have the cid, we try getting the fedfind release using the cid instead, and return that if it works. Upshot being, we can once again work with the `ComposeEvent` for a new candidate compose before it's even imported to PDC, and find its images, so long as we instantiate it from its compose ID. The other change is to introduce another fedfind property, `ff_release_images`. This one only returns a fedfind release instance if it can find one that actually has images; otherwise it raises a new error, FedfindNotFoundError. This is partly used just to implement the 'skip download page creation with a warning if compose cannot be found' behaviour described above, but it will also cover another (small) potential corner cases with the candidate compose syncing: if the synced Compose 'exists' so far as fedfind is concerned, but doesn't yet contain any images - which could potentially at least occur during sync - we will now catch this, and use the Production instance instead to create the download page. Finally, we extend the test suite quite significantly to cover a lot of this ground. Signed-off-by: Adam Williamson --- diff --git a/tests/test_event.py b/tests/test_event.py new file mode 100644 index 0000000..263f617 --- /dev/null +++ b/tests/test_event.py @@ -0,0 +1,226 @@ +# Copyright (C) 2016 Red Hat +# +# This file is part of wikitcms. +# +# wikitcms is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# +# Author: Adam Williamson + +# these are all kinda inappropriate for pytest patterns +# pylint: disable=old-style-class, no-init, protected-access, no-self-use, unused-argument +# pylint: disable=invalid-name + +"""Tests for event.py.""" + +from __future__ import unicode_literals +from __future__ import print_function + +import fedfind.release +# note: we need the external 'mock', unittest.mock does not seem to +# work correctly with the @patch decorator still +import mock +import pytest +import wikitcms.wiki as wk +import wikitcms.event +from wikitcms.exceptions import FedfindNotFoundError + +def fakemwpinit(self, site, name, *args, **kwargs): + """Stub init for mwclient.Page, we can't just mock it out as we + need to set site and name. + """ + self.site = site + self.name = name + +class TestEventFedfind: + """Tests related to fedfind release discovery from validation + events. + """ + # the 'fjajah' is just to make sure we're running offline; if I + # screw up and add a test that hits the network it'll cause the + # tests to hang/fail instead of succeeding a bit slower + site = wk.Wiki('fjajah', do_init=False, force_login=False) + + @mock.patch('fedfind.release.Compose.exists', True) + @mock.patch('fedfind.release.Compose.all_images', ['foo']) + def test_candidate_ff_release_compose(self): + """Straightforward ff_release test case for a candidate + compose which is properly synced to stage. Should work the + same whether or not we pass the cid hint, both properties + should exist and be the Compose instance. + """ + event = wikitcms.event.ComposeEvent( + self.site, '27', 'RC', '1.6', cid='Fedora-27-20171105.0') + assert isinstance(event.ff_release, fedfind.release.Compose) + assert isinstance(event.ff_release_images, fedfind.release.Compose) + event = wikitcms.event.ComposeEvent(self.site, '27', 'RC', '1.6') + assert isinstance(event.ff_release, fedfind.release.Compose) + assert isinstance(event.ff_release_images, fedfind.release.Compose) + + @mock.patch('fedfind.helpers.cid_from_label', return_value='') + @mock.patch('fedfind.release.Compose.exists', False) + @mock.patch('fedfind.release.Production.all_images', ['foo']) + @mock.patch('fedfind.release.Production.cid', 'Fedora-27-20171105.0') + def test_candidate_ff_release_compose_gap(self, fakecidfromlabel): + """Test the 'compose gap' case: this occurs when a candidate + compose has just been created, but not yet synced to stage, + and also has not yet appeared in PDC. In this case, without + the 'cid' hint, we will not be able to find the fedfind + release associated with the event. With the hint, we should + find it in the non-synced location (as a fedfind Production). + """ + event = wikitcms.event.ComposeEvent( + self.site, '27', 'RC', '1.6', cid='Fedora-27-20171105.0') + assert isinstance(event.ff_release, fedfind.release.Production) + assert isinstance(event.ff_release_images, fedfind.release.Production) + event = wikitcms.event.ComposeEvent(self.site, '27', 'RC', '1.6') + with pytest.raises(FedfindNotFoundError): + print(event.ff_release) + with pytest.raises(FedfindNotFoundError): + print(event.ff_release_images) + + @mock.patch('fedfind.helpers.cid_from_label', return_value='Fedora-27-20171105.0') + @mock.patch('fedfind.release.Compose.exists', False) + @mock.patch('fedfind.release.Production.all_images', ['foo']) + @mock.patch('fedfind.release.Production.cid', 'Fedora-27-20171105.0') + def test_candidate_ff_release_compose_gap_pdc(self, fakecidfromlabel): + """Test the case where the candidate compose has not yet + synced to stage, but has appeared in PDC. In this case, we + should find the ff_release in the non-synced location (as a + fedfind Production) with or without the cid hint. + """ + event = wikitcms.event.ComposeEvent( + self.site, '27', 'RC', '1.6', cid='Fedora-27-20171105.0') + assert isinstance(event.ff_release, fedfind.release.Production) + assert isinstance(event.ff_release_images, fedfind.release.Production) + event = wikitcms.event.ComposeEvent(self.site, '27', 'RC', '1.6') + assert isinstance(event.ff_release, fedfind.release.Production) + assert isinstance(event.ff_release_images, fedfind.release.Production) + + @mock.patch('fedfind.release.Compose.exists', True) + @mock.patch('fedfind.release.Compose.all_images', []) + @mock.patch('fedfind.release.Production.all_images', ['foo']) + @mock.patch('fedfind.release.Production.cid', 'Fedora-27-20171105.0') + def test_candidate_ff_release_compose_exists_no_images(self): + """Test a potential tricky case where a candidate compose + tree exists on stage but the images haven't shown up in it + yet. With the cid hint, the event's ff_release should be the + Compose instance, but its ff_release_images should be the + Production instance. Without the hint, we won't get images. + """ + event = wikitcms.event.ComposeEvent( + self.site, '27', 'RC', '1.6', cid='Fedora-27-20171105.0') + assert isinstance(event.ff_release, fedfind.release.Compose) + assert isinstance(event.ff_release_images, fedfind.release.Production) + event = wikitcms.event.ComposeEvent(self.site, '27', 'RC', '1.6') + assert isinstance(event.ff_release, fedfind.release.Compose) + with pytest.raises(FedfindNotFoundError): + assert event.ff_release_images + + @mock.patch('fedfind.release.BranchedNightly.exists', True) + @mock.patch('fedfind.release.BranchedNightly.all_images', ['foo']) + def test_candidate_ff_release_nightly(self): + """Straightforward ff_release test case for a nightly + compose which exists and has images. + """ + event = wikitcms.event.NightlyEvent(self.site, '27', 'Branched', '20171104.n.0') + assert isinstance(event.ff_release, fedfind.release.BranchedNightly) + assert isinstance(event.ff_release_images, fedfind.release.BranchedNightly) + + @mock.patch('fedfind.release.BranchedNightly.exists', False) + @mock.patch('fedfind.release.BranchedNightly.all_images', []) + def test_candidate_ff_release_nightly_no_images(self): + """ff_release test case for a nightly compose which doesn't + exist and has no images. We get ff_release (as fedfind doesn't + do an existence check in this case), but not images. + """ + event = wikitcms.event.NightlyEvent(self.site, '27', 'Branched', '20171104.n.0') + assert isinstance(event.ff_release, fedfind.release.BranchedNightly) + with pytest.raises(FedfindNotFoundError): + assert event.ff_release_images + +@mock.patch('wikitcms.page.mwp.Page.__init__', fakemwpinit) +@mock.patch('wikitcms.page.Page.save', autospec=True) +@mock.patch('wikitcms.page.ValidationPage.update_current', autospec=True) +@mock.patch('wikitcms.event.ValidationEvent.update_current', autospec=True) +@mock.patch('test_event.wk.Wiki.testtypes', ['Installation', 'Base', 'Server', 'Cloud', 'Desktop']) +class TestEventCreate: + """Tests related to event creation.""" + site = wk.Wiki('fjajah', do_init=False, force_login=False) + + @mock.patch('fedfind.release.BranchedNightly.exists', True) + @mock.patch('fedfind.release.BranchedNightly.all_images', + [{ + 'arch': 'x86_64', + 'format': 'iso', + 'path': ('Workstation/x86_64/iso/' + 'Fedora-Workstation-Live-x86_64-27-2011104.n.0.iso'), + 'subvariant': 'Workstation', + 'type': 'live', + 'url': ('https://kojipkgs.fedoraproject.org/compose/branched/' + 'Fedora-27-20171104.n.0/Workstation/x86_64/iso/' + 'Fedora-Workstation-Live-x86_64-27-2011104.n.0.iso'), + }]) + def test_event_create(self, fakeevup, fakepageup, fakepagesave): + """Test normal event creation.""" + event = wikitcms.event.NightlyEvent(self.site, '27', 'Branched', '20171104.n.0') + event.create() + # we should save 5 test pages, plus the summary page, + # download page and two category pages + assert fakepagesave.call_count == 9 + # we should call update_current for all 5 test pages + assert fakepageup.call_count == 5 + # we should call update_current for the event itself + assert fakeevup.call_count == 1 + + @mock.patch('fedfind.release.BranchedNightly.exists', False) + @mock.patch('fedfind.release.BranchedNightly.all_images', []) + def test_event_create_no_images(self, fakeevup, fakepageup, fakepagesave): + """Test event creation where no images are available. This + should succeed, but not create a download page. + """ + event = wikitcms.event.NightlyEvent(self.site, '27', 'Branched', '20171104.n.0') + event.create() + # we should save 5 test pages, plus the summary page and + # two category pages - but no download page + assert fakepagesave.call_count == 8 + # we should call update_current for all 5 test pages + assert fakepageup.call_count == 5 + # we should call update_current for the event itself + assert fakeevup.call_count == 1 + + @mock.patch('wikitcms.page.mwp.Page.text', return_value="sometext") + @mock.patch('fedfind.release.BranchedNightly.exists', False) + @mock.patch('fedfind.release.BranchedNightly.all_images', []) + def test_event_create_check(self, fakepagetext, fakeevup, fakepageup, fakepagesave): + """Test event creation when pages already exist and check is + True. We should raise an error in this case. Using the 'no + download page' case because the mocks are shorter. + """ + event = wikitcms.event.NightlyEvent(self.site, '27', 'Branched', '20171104.n.0') + with pytest.raises(ValueError): + event.create(check=True) + + @mock.patch('fedfind.release.BranchedNightly.exists', False) + @mock.patch('fedfind.release.BranchedNightly.all_images', []) + def test_event_create_testtypes(self, fakeevup, fakepageup, fakepagesave): + """Test event creation for a specified set of test types.""" + event = wikitcms.event.NightlyEvent(self.site, '27', 'Branched', '20171104.n.0') + event.create(testtypes=['Installation', 'Server']) + # we should save 2 test pages, plus the summary page and + # two category pages - but no download page + assert fakepagesave.call_count == 5 + # we should call update_current for both test pages + assert fakepageup.call_count == 2 + # we should call update_current for the event itself + assert fakeevup.call_count == 1 diff --git a/tests/test_wiki.py b/tests/test_wiki.py index 5a13edf..c46817f 100644 --- a/tests/test_wiki.py +++ b/tests/test_wiki.py @@ -98,21 +98,26 @@ class TestWiki: self.site.add_to_category('Foobar', 'Category:Some category', 'summary') fakesave.assert_called_with('foobar\n[[Category:Some category]]', 'summary', createonly=False) + @mock.patch('fedfind.release.Compose.exists', return_value=True) + @mock.patch('fedfind.release.Production.label', 'RC-1.6') + @mock.patch('fedfind.release.Production.cid', 'Fedora-27-20171105.0') + @mock.patch('fedfind.helpers.get_current_release', autospec=True, return_value=27) @mock.patch('mwclient.page.Page.__init__', return_value=None) @mock.patch('mwclient.page.Page.text', return_value=FAKE_CURRENT_COMPOSE) @mock.patch('wikitcms.event.NightlyEvent', autospec=True) @mock.patch('wikitcms.event.ComposeEvent', autospec=True) - def test_get_validation_event(self, fakecompose, fakenightly, faketext, fakeinit): + def test_get_validation_event(self, fakecompose, fakenightly, faketext, fakeinit, + fakegetcurr, fakecompexists): # current event ret = self.site.get_validation_event() - fakecompose.assert_called_with(self.site, '24', 'Alpha', '1.1', modular=False) + fakecompose.assert_called_with(self.site, '24', 'Alpha', '1.1', modular=False, cid='') ret = self.site.get_validation_event(modular=True) - fakecompose.assert_called_with(self.site, '24', 'Alpha', '1.1', modular=True) + fakecompose.assert_called_with(self.site, '24', 'Alpha', '1.1', modular=True, cid='') # old-school TC/RC ret = self.site.get_validation_event(23, 'Final', 'TC9') - fakecompose.assert_called_with(self.site, 23, 'Final', 'TC9', modular=False) + fakecompose.assert_called_with(self.site, 23, 'Final', 'TC9', modular=False, cid='') ret = self.site.get_validation_event(23, 'Beta', 'RC1') - fakecompose.assert_called_with(self.site, 23, 'Beta', 'RC1', modular=False) + fakecompose.assert_called_with(self.site, 23, 'Beta', 'RC1', modular=False, cid='') # old-school nightly ret = self.site.get_validation_event(23, 'Rawhide', '20151112') fakenightly.assert_called_with( @@ -122,12 +127,12 @@ class TestWiki: self.site, release=23, milestone='Branched', compose='20151211', modular=False) # Pungi 4 production/candidate ret = self.site.get_validation_event(24, 'Alpha', '1.1') - fakecompose.assert_called_with(self.site, 24, 'Alpha', '1.1', modular=False) + fakecompose.assert_called_with(self.site, 24, 'Alpha', '1.1', modular=False, cid='') ret = self.site.get_validation_event(27, 'Beta', '1.5', modular=True) - fakecompose.assert_called_with(self.site, 27, 'Beta', '1.5', modular=True) + fakecompose.assert_called_with(self.site, 27, 'Beta', '1.5', modular=True, cid='') # Past 23, 'Final' milestone should be converted to 'RC' ret = self.site.get_validation_event(25, 'Final', '1.1') - fakecompose.assert_called_with(self.site, 25, 'RC', '1.1', modular=False) + fakecompose.assert_called_with(self.site, 25, 'RC', '1.1', modular=False, cid='') # Pungi 4 nightly ret = self.site.get_validation_event(24, 'Rawhide', '20160222.n.0') fakenightly.assert_called_with( @@ -138,6 +143,18 @@ class TestWiki: ret = self.site.get_validation_event(27, 'Branched', '20171110.n.1', modular=True) fakenightly.assert_called_with( self.site, release=27, milestone='Branched', compose='20171110.n.1', modular=True) + # Rawhide nightly compose ID + ret = self.site.get_validation_event(cid='Fedora-Rawhide-20180220.n.0', modular=False) + fakenightly.assert_called_with( + self.site, release='28', milestone='Rawhide', compose='20180220.n.0', modular=False) + # Branched nightly compose ID + ret = self.site.get_validation_event(cid='Fedora-27-20171120.n.0', modular=False) + fakenightly.assert_called_with( + self.site, release='27', milestone='Branched', compose='20171120.n.0', modular=False) + # Candidate compose ID (note compose ID passthrough) + ret = self.site.get_validation_event(cid='Fedora-27-20171105.0', modular=False) + fakecompose.assert_called_with( + self.site, '27', 'RC', '1.6', modular=False, cid='Fedora-27-20171105.0') with pytest.raises(ValueError): # Non-nightly compose but no milestone diff --git a/wikitcms/event.py b/wikitcms/event.py index 9d9ad17..b16f4fd 100644 --- a/wikitcms/event.py +++ b/wikitcms/event.py @@ -33,6 +33,7 @@ from cached_property import cached_property from . import listing from . import page from . import helpers +from wikitcms.exceptions import FedfindNotFoundError logger = logging.getLogger(__name__) @@ -143,17 +144,36 @@ class ValidationEvent(object): """A fedfind release object matching this event.""" # note: fedfind has a hack that parses date and respin out # of a dot-separated compose, since wikitcms smooshes them - # into the compose value. Also note that *this will fail* - # for ComposeEvents in the short time after the relevant - # compose is finished but before it is imported to PDC and/or - # synced to stage. + # into the compose value. dist = "Fedora" if self.modular: dist = "Fedora-Modular" - return fedfind.release.get_release(release=self.release, - milestone=self.milestone, - compose=self.compose, - dist=dist) + try: + return fedfind.release.get_release(release=self.release, + milestone=self.milestone, + compose=self.compose, + dist=dist) + except ValueError as err: + try: + if self._cid: + return fedfind.release.get_release(cid=self._cid) + except AttributeError: + raise FedfindNotFoundError(err) + raise FedfindNotFoundError(err) + + @property + def ff_release_images(self): + """A fedfind release object matching this event, that has + images. If we can't find one, raise an exception. For the + base class this just acts as a check on ff_release; it does + something more clever in ComposeEvent. + """ + rel = self.ff_release + if rel.all_images: + return rel + else: + raise FedfindNotFoundError("Could not find fedfind release with images for event" + "{0}".format(self.version)) def update_current(self): """Make the CurrentFedoraCompose template on the wiki point to @@ -201,11 +221,13 @@ class ValidationEvent(object): if any(pag.text() for pag in pages): raise ValueError("A result page already exists!") - # NOTE: download page creation for ComposeEvents ultimately - # relies on the compose either being synced to stage or being - # imported to PDC. If you try to create a ComposeEvent before - # the compose is imported to PDC or synced to stage, it will - # not go well. relvalconsumer waits for PDC. + # NOTE: download page creation for ComposeEvents will only + # work if: + # * the compose has being synced to stage, OR + # * the compose has been imported to PDC, OR + # * you used get_validation_event and passed it a cid + # Otherwise, the event will be created, but the download page + # will not. pages.extend((self.summary_page, self.download_page, self.category_page, self.parent_category_page)) @@ -229,6 +251,11 @@ class ValidationEvent(object): except AttributeError: # not all pages have update_current. this is fine. pass + except FedfindNotFoundError: + # this happens if download page couldn't be created + # because fedfind release couldn't be found + logger.warning("Could not create download page for event %s as fedfind release " + "was not found!") if current: try: self.update_current() @@ -249,9 +276,14 @@ class ComposeEvent(ValidationEvent): the testing for a particular nightly, test compose or release candidate build. """ - def __init__(self, site, release, milestone, compose, modular=False): + def __init__(self, site, release, milestone, compose, modular=False, cid=''): super(ComposeEvent, self).__init__( site, release, milestone=milestone, compose=compose, modular=modular) + # this is a little hint that gets set via get_validation_event + # when getting a page or event by cid; it helps us find the + # fedfind release for the event if the compose is not yet in + # PDC or synced to stage + self._cid = cid self.shortver = "{0} {1}".format(self.milestone, self.compose) @property @@ -290,6 +322,30 @@ class ComposeEvent(ValidationEvent): # event doesn't exist return "" + @property + def ff_release_images(self): + """A fedfind release object matching this event, that has + images. If we can't find one, raise an exception. Here, we + try getting the release by (release, milestone, compose), but + if that release has no images - which happens in the specific + case that we've just created an event for a candidate compose + which has not yet been synced to stage - and we have the cid + hint, we try getting a release by cid instead, which should + find the compose in kojipkgs (a fedfind Production rather than + Compose). + """ + rel = self.ff_release + if rel.all_images: + return rel + + if self._cid: + rel = fedfind.release.get_release(cid=self._cid) + if rel.all_images: + return rel + else: + raise FedfindNotFoundError("Could not find fedfind release with images for event " + "{0}".format(self.version)) + class NightlyEvent(ValidationEvent): """An Event that describes a release validation event - that is, diff --git a/wikitcms/exceptions.py b/wikitcms/exceptions.py index 736bcd0..6c6c3a3 100644 --- a/wikitcms/exceptions.py +++ b/wikitcms/exceptions.py @@ -22,16 +22,28 @@ from __future__ import unicode_literals from __future__ import print_function + class NoPageError(Exception): - """Page does note exist.""" + """Page does not exist.""" pass + class NotFoundError(Exception): """Requested thing wasn't found.""" pass + class TooManyError(Exception): """Found too many of the thing you asked for.""" pass + +# this inherits from ValueError as the things that raise this may +# previously have passed along a ValueError from fedfind +class FedfindNotFoundError(ValueError, NotFoundError): + """Couldn't find a fedfind release (probably the fedfind release + that matches an event). + """ + pass + # vim: set textwidth=100 ts=8 et sw=4: diff --git a/wikitcms/page.py b/wikitcms/page.py index 8255116..3e4e848 100644 --- a/wikitcms/page.py +++ b/wikitcms/page.py @@ -38,7 +38,7 @@ from mwclient import page as mwp from . import result as rs from . import helpers -from wikitcms.exceptions import NoPageError, NotFoundError, TooManyError +from wikitcms.exceptions import NoPageError, NotFoundError, TooManyError, FedfindNotFoundError logger = logging.getLogger(__name__) @@ -537,7 +537,7 @@ class DownloadPage(Page): # encounter (that's arches). arches = set() imagedict = dict() - for img in self.event.ff_release.all_images: + for img in self.event.ff_release_images.all_images: if img['arch']: arches.add(img['arch']) # simple human-readable identifier for the image @@ -586,8 +586,7 @@ class DownloadPage(Page): for img in imgs: if img['arch'] == arch: # Add a link to the image if we have one - url = '/'.join((self.event.ff_release.location, - img['path'])) + url = '/'.join((self.event.ff_release_images.location, img['path'])) table += '[{0} Download]'.format(url) table += '\n' # Close out the table when we're done diff --git a/wikitcms/wiki.py b/wikitcms/wiki.py index 7306b31..10a776e 100644 --- a/wikitcms/wiki.py +++ b/wikitcms/wiki.py @@ -415,7 +415,7 @@ class Wiki(mwclient.Site): # be nice and convert it if int(release) > 23 and milestone.lower() == 'final': milestone = 'RC' - return ev.ComposeEvent(self, release, milestone, compose, modular=modular) + return ev.ComposeEvent(self, release, milestone, compose, modular=modular, cid=cid) else: # We should never get here, but just in case. raise ValueError(