Fix editing multi-build updates when title failed to update #2435
Conversation
puiterwijk
added
Critical
puiterwijk
force-pushed
the
fixmultiupdates
branch
3 times, most recently
from
26c3f7b to
edeb1db
Compare
| "Update for {} already exists".format(nvr)) | ||
| # Ensure it doesn't already exist in another update | ||
| build = request.db.query(Build).filter_by(nvr=nvr).first() | ||
| if build and build.update is not None and up.title != build.update.title: |
There was a problem hiding this comment.
Is the intention here to compare up to build.update? If so, it might be good to compare the alias field instead of the title field since we want to remove/repurpose the title field some day.
There was a problem hiding this comment.
Also, I'm not sure, but you might be able to use the is operator to compare them. I'd have to read some sqlalchemy docs to verify. If it were me, I'd just play it safe and compare a field I know to be unique like you did here.
There was a problem hiding this comment.
Sure, I'll switch to the .alias field.
However, the whole problem is that sometimes the update's title doesn't get correctly updated, and on later update, if one of the builds in the update remain, it will abort otherwise.
My example from the test:
- I submitted
a-1.0-1.fc17 b-1.0-1.fc17 c-1.0-1.fc17 - At some point, I edited that to replace
b-1.0-1.fc17withb-1.0-2.fc17. However, due to a sometimes-occuring bug, theupdate.titleis stilla-1.0-1.fc17 b-1.0-1.fc17 c-1.0-1.fc17. This has happened a few times, and given that we're intending to remove/repurpose title, I didn't feel like fixing that. - I then want to edit it to replace
c-1.0-1.fc17withc-2.0-1.fc17. At this point, Bodhi checks each of the new builds whether they are in an existing update that is not the update I'm testing. It does that by testing against thenvr in update.title. HOWEVER, since the title didn't update correctly after the first edit, it no longer seesb-1.0-2.fc17as part of this update, but when it goes request that build, that build does have an update attached to it. - My last edit now fails, because
b-1.0-2.fc17 is in another update.
This patch fixes that problem by instead of checking whether all of the new builds are listed in the update.title, just look if the update the build is in is the one we're currently editing, thus fixing the example.
There was a problem hiding this comment.
That makes sense, thanks for the detailed explanation!
| @@ -227,13 +227,11 @@ def validate_builds(request, **kwargs): | |||
| 'Cannot edit stable updates') | |||
|
|
|||
| for nvr in request.validated.get('builds', []): | |||
| # If the build is new | |||
| if nvr not in edited: | |||
There was a problem hiding this comment.
Do you think this was here just to help with performance? It seems like it was just trying to make this verification when an NVR was being added to an update. Do you think we need to always perform this test?
|
|
||
| Removed build(s): | ||
|
|
||
| - b-1.0-1.fc17 |
There was a problem hiding this comment.
One day I will add tags to the Comment model so we don't have to have Bodhi test its own comments to see if stuff like this happened :)
| args['builds'] = ['a-1.0-1.fc17', 'b-2.0-1.fc17', 'c-1.0-1.fc17'] | ||
| r = self.app.post_json('/updates/', args) | ||
| up = r.json_body | ||
| self.assertEquals(up['title'], u'a-1.0-1.fc17 b-2.0-1.fc17 c-1.0-1.fc17') |
There was a problem hiding this comment.
The name and description of the test makes it sound like we expect the title not to change, but it seems like the title is correct here.
There was a problem hiding this comment.
This catches the case where the title didn't correctly update on an earlier update, read my summary from the other comment.
Sometimes an update title fails to update, and the title was used in multi-build updates to determine whether a particular build was in the update being edited. This patch makes it actually look at whether the build is in the current update by comparing. Signed-off-by: Patrick Uiterwijk <[email protected]>
bowlofeggs
force-pushed
the
fixmultiupdates
branch
from
edeb1db to
fe7cd59
Compare
|
This patch is planned to be included in the upcoming 3.10.0 release: #2556. |
|
This patch has been deployed to Fedora's staging Bodhi instance: |
Sometimes an update title fails to update, and the title was used
in multi-build updates to determine whether a particular build was
in the update being edited.
This patch makes it actually look at whether the build is in the
current update by comparing.
Signed-off-by: Patrick Uiterwijk [email protected]