Differential D1179

Bring back `clean_updates`, have it delete updates from DB
ClosedPublic
Actions

Authored by adamwill on Mar 25 2017, 12:53 AM.

Details

Reviewers
tflink
mkrizek
Commits
rBLKR58401a5ce188: Bring back `clean_updates`, have it delete updates from DB
Summary

This is a modified revert of commit
2da425e354904273793e5321030882aac98d0c39.

Turns out this wasn't actually just cleaning up *deleted*
updates, though that's what it clearly thought it was doing.
It also marked updates for closed bugs - and other updates
which were no longer associated with a blocker or FE update,
for instance if the update was edited and no longer marked as
fixing the blocker bug - as 'deleted'. This is clearly wrong,
but did more or less the right thing sort of by accident; it
resulted in these updates not being shown in any of the views.

None of the 'update' code will actually update the status of
updates which no longer relate to a blocker or FE bug, so after
we removed clean_updates, there was a problem where updates
for closed bugs were still listed in the database with status
'testing', even though they're actually 'stable'. This probably
doesn't affect the main parts of the web UI much, since they
only actually show updates that fix bugs, but it does affect
the new 'requests' tab - it results in it including updates that
have already gone stable in both the 'compose request' and the
'stable push request' texts.

We could deal with this by tweaking the update code so we update
the status for all updates in the database, I guess, but I kinda
like this idea to trim the database by dropping updates that no
longer relate to blocker bugs from it entirely. If for whatever
reason an update pongs *back* to being relevant, it'll just get
re-added to the database at the next sync.

Test Plan

Bit tricky, as you need to get an update into the
database that subsequently becomes 'irrelevant' to see the
behaviour. If you just build a database while we're in an active
cycle then wait a few days, till after some of the bugs get
closed, that should work. Without this patch, the update will
likely remain in the database with state 'testing', and show
up in the 'requests' tab.

Diff Detail

Repository
rBLKR blockerbugs
Branch
clean-updates (branched from develop)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1115
Build 1115: arc lint + arc unit
adamwill created this revision.Mar 25 2017, 12:53 AM
adamwill updated this revision to Diff 3000.Mar 25 2017, 1:14 AM

Fix and extend test

mkrizek accepted this revision.Mar 27 2017, 8:31 AM

Didn't test, but code looks good to me.

This revision is now accepted and ready to land.Mar 27 2017, 8:31 AM
adamwill closed this revision.Mar 27 2017, 3:52 PM

Revision Contents

Diff 3000

View Options

blockerbugs/util/update_sync.py

Show First 20 Lines • Show All 88 Lines • ▼ Show 20 Line(s)34class UpdateSync(object):
89 89​
90 def is_watched_bug(self, bugnum): 90​ def is_watched_bug(self, bugnum):
91 watched_bug = Bug.query.filter_by(bugid=bugnum).first() 91​ watched_bug = Bug.query.filter_by(bugid=bugnum).first()
92 if watched_bug: 92​ if watched_bug:
93 return True 93​ return True
94 else: 94​ else:
95 return False 95​ return False
96 96​
97​ def clean_updates(self, updates, relid):
98​ """Remove updates for this release which are no longer related
99​ to any blocker or freeze exception bug from the database.
100​ """
101​ query_updates = Update.query.filter(
102​ Update.release_id == relid,
103​ ).all()
104​ db_updates = set(update.title for update in query_updates)
105​ bodhi_updates = set(update['title'] for update in updates)
106​ unneeded_updates = db_updates.difference(bodhi_updates)
107​
108​ for update in query_updates:
109​ if update.title in unneeded_updates:
110​ self.log.debug("Removing no longer relevant update %s" %
111​ update.title)
112​ self.db.session.delete(update)
113​ self.db.session.commit()
114​
97 def search_updates(self, bugids, release_num): 115​ def search_updates(self, bugids, release_num):
98 queries_data = dict( 116​ queries_data = dict(
99 bugs=[str(bug_id) for bug_id in bugids], 117​ bugs=[str(bug_id) for bug_id in bugids],
100 release='f%d' % release_num, 118​ release='f%d' % release_num,
101 limit=100, 119​ limit=100,
102 ) 120​ )
103 try: 121​ try:
104 result = self.bodhi_interface.query(**queries_data) 122​ result = self.bodhi_interface.query(**queries_data)
Show All 37 Lines156 def sync_bug_updates(self, release, bugs, fullsync=False):
142 try: 160​ try:
143 updates = self.search_updates(bugs_ids, release.number) 161​ updates = self.search_updates(bugs_ids, release.number)
144 except ServerError, ex: 162​ except ServerError, ex:
145 self.log.error( 163​ self.log.error(
146 'f{r.number} sync updates failed: {e.code} {e.msg}'.format( 164​ 'f{r.number} sync updates failed: {e.code} {e.msg}'.format(
147 e=ex, r=release)) 165​ e=ex, r=release))
148 return 166​ return
149 167​
168​ # remove no longer relevant updates from the database
169​ self.clean_updates(updates, release.id)
170​
150 for update in updates: 171​ for update in updates:
151 self.log.debug('running sync for update %s' % update['title']) 172​ self.log.debug('running sync for update %s' % update['title'])
152 existing_update = Update.query.filter_by( 173​ existing_update = Update.query.filter_by(
153 title=update['title']).first() 174​ title=update['title']).first()
154 if existing_update: 175​ if existing_update:
155 self.log.debug( 176​ self.log.debug(
156 'syncing existing update %s' % existing_update.title) 177​ 'syncing existing update %s' % existing_update.title)
157 existing_update.sync(update, fullsync) 178​ existing_update.sync(update, fullsync)
Show All 20 Lines
View Options

testing/testfunc_update_sync.py

Show First 20 Lines • Show All 187 Lines • ▼ Show 20 Line(s)90class TestfuncUpdateSync(object):
188 def test_release_server_error(self): 188​ def test_release_server_error(self):
189 add_bug(4000, 'testbug1', self.test_milestone99alpha) 189​ add_bug(4000, 'testbug1', self.test_milestone99alpha)
190 update_sync = UpdateSync(db, FakeRaisesBodhiInterface) 190​ update_sync = UpdateSync(db, FakeRaisesBodhiInterface)
191 log_error = Mock(wraps=update_sync.log.error) 191​ log_error = Mock(wraps=update_sync.log.error)
192 update_sync.log.error = log_error 192​ update_sync.log.error = log_error
193 update_sync.sync_updates(self.test_release99) 193​ update_sync.sync_updates(self.test_release99)
194 assert log_error.call_count == 1 194​ assert log_error.call_count == 1
195 assert 'Internal Server Error' in log_error.call_args[0][0] 195​ assert 'Internal Server Error' in log_error.call_args[0][0]
196​
197​ def test_sync_clean_updates(self):
198​ bug1 = add_bug(3000, 'testbug1', self.test_milestone99alpha)
199​
200​ self.update_sync.sync_updates(self.test_release99)
201​ updates = Update.query.all()
202​ # by default we have two updates for bug #3000
203​ assert len(updates) == 2
204​
205​ # now we do a sync with only *one* update for bug #3000
206​ b = FakeBodhiInterface
207​ b.updates[('f99', '3000')] = [update1_for_bug_3000]
208​ update_sync = UpdateSync(db, b)
209​ update_sync.sync_updates(self.test_release99)
210​
211​ # so now we should have just *one* update in the db, the other
212​ # update should disappear. This is testing the (no longer
213​ # likely) case where an update disappears entirely from Bodhi
214​ updates = Update.query.all()
215​ assert len(updates) == 1
216​
217​ # Now let's mark the bug as closed and sync again.
218​ bug1.active = False
219​ add_bug(4000, 'testbug2', self.test_milestone99alpha)
220​ self.update_sync.sync_updates(self.test_release99)
221​
222​ # Now the other update should be removed from the db, as it
223​ # no longer relates to an open blocker/FE bug
224​ updates = Update.query.all()
225​ assert len(updates) == 0