#137 sync_updates: allow Bugs for same bug ID but different milestone
Merged 3 years ago by kparal. Opened 3 years ago by adamwill.
fedora-qa/ adamwill/blockerbugs update-bug-milestones  into  develop

file modified
+2 -4
@@ -193,12 +193,10 @@ 

  

  

  def sync_updates(args):

-     fullsync = args.full

- 

      active_releases = Release.query.filter_by(active=True).all()

      update_sync = UpdateSync(db)

      for release in active_releases:

-         update_sync.sync_updates(release, fullsync)

+         update_sync.sync_updates(release)

  

  

  def sync_discussions(args):
@@ -280,7 +278,7 @@ 

                                                  help='Synchronize updates only')

      sync_updates_parser.add_argument('-f', '--full',

                                       action='store_true', default=False,

-                                      help='Force full sync')

+                                      help='Deprecated, no longer does anything')

      sync_updates_parser.set_defaults(func=sync_updates)

  

      sync_discussions_parser = subparsers.add_parser('sync-discussions',

file modified
+9 -10
@@ -85,7 +85,7 @@ 

      def __str__(self):

          return 'update: %s' % (self.title)

  

-     def sync(self, updateinfo, fullsync=False):

+     def sync(self, updateinfo):

          self.title = updateinfo['title']

          self.url = updateinfo['url']

          self.karma = updateinfo['karma']
@@ -98,16 +98,15 @@ 

          if updateinfo['date_pushed_stable']:

              self.date_pushed_stable = updateinfo['date_pushed_stable']

          self.pending = updateinfo['pending']

-         current_bugids = [bug.bugid for bug in self.bugs]

+         current_bugkeys = [(bug.bugid, bug.milestone) for bug in self.bugs]

          for bugid in updateinfo['bugs']:

-             # if full sync is specified, we want to sync all of the bugs

-             if fullsync or not bugid in current_bugids:

-                 newbugs = Bug.query.filter_by(bugid=bugid).all()

-                 for bug in newbugs:

-                     if not bugid in current_bugids:

-                         self.bugs.append(bug)

-                     if bug.milestone and not bug.milestone in self.milestones:

-                         self.milestones.append(bug.milestone)

+             newbugs = Bug.query.filter_by(bugid=bugid).all()

+             for bug in newbugs:

+                 if not (bug.bugid, bug.milestone) in current_bugkeys:

+                     self.bugs.append(bug)

+                     current_bugkeys.append((bug.bugid, bug.milestone))

+                 if bug.milestone and not bug.milestone in self.milestones:

+                     self.milestones.append(bug.milestone)

  

      @classmethod

      def from_data(cls, updateinfo, release):

@@ -175,7 +175,7 @@ 

              buglist.extend(milestone.bugs.filter_by(active=True).all())

          return buglist

  

-     def sync_bug_updates(self, release, bugs, fullsync=False):

+     def sync_bug_updates(self, release, bugs):

          starttime = datetime.utcnow()

          bugs_ids = [bug.bugid for bug in bugs]

          self.log.debug('searching for updates for bugs %s' % str(bugs_ids))
@@ -197,7 +197,7 @@ 

              if existing_update:

                  self.log.debug(

                      'syncing existing update %s' % existing_update.title)

-                 existing_update.sync(update, fullsync)

+                 existing_update.sync(update)

              else:

                  self.log.debug('creating new update %s' % update['title'])

                  existing_update = Update.from_data(update, release)
@@ -208,7 +208,7 @@ 

          self.db.session.add(release)

          self.db.session.commit()

  

-     def sync_updates(self, release, fullsync=False):

+     def sync_updates(self, release):

          bugs = self.get_release_bugs(release)

          self.log.info(

              'found %d bugs in f%d release' % (len(bugs), release.number))
@@ -217,4 +217,4 @@ 

                  'no bugs in f%d release, skip update' % (release.number))

              release.last_update_sync = datetime.utcnow()

              return

-         self.sync_bug_updates(release, bugs, fullsync)

+         self.sync_bug_updates(release, bugs)

@@ -117,10 +117,14 @@ 

          db.create_all()

          self.test_release99 = Release(99)

          self.test_milestone99alpha = Milestone(

-             self.test_release99, 'alpha', 1000, 1001, '99-final')

+             self.test_release99, 'alpha', 1000, 1001, '99-alpha')

          self.test_milestone99alpha.active = True

+         self.test_milestone99beta = Milestone(

+             self.test_release99, 'beta', 1002, 1003, '99-beta')

+         self.test_milestone99beta.active = True

          db.session.add(self.test_release99)

          db.session.add(self.test_milestone99alpha)

+         db.session.add(self.test_milestone99beta)

          db.session.commit()

          self.update_sync = UpdateSync(db, FakeBodhiInterface)

  
@@ -183,6 +187,33 @@ 

          bugs_id = set(bug.bugid for update in updates for bug in update.bugs)

          assert bugs_id == set((bug1.bugid, bug2.bugid, bug3.bugid))

  

+     def test_sync_bug_with_two_milestones(self):

+         """Check that when a bug is Beta AcceptedFreezeException and

+         Final AcceptedBlocker, the update is associated with *two*

+         Bugs - both for the same bug ID, but one for Beta milestone

+         and one for Final milestone.

+         """

+         bug1 = add_bug(2000, 'testbug1', self.test_milestone99alpha)

+         bug1.accepted_blocker = False

+         bug1.accepted_fe = True

+         # we add the first Bug, sync, then add the second Bug and sync

+         # again because that's the hardest case to handle. This

+         # represents the real-world scenario of "bug is accepted as a

+         # Beta FE, we run a sync, then later bug is accepted as Final

+         # blocker and we sync again". Before this test was added,

+         # sync_updates would DTRT if both the "Beta accepted FE" Bug

+         # and "Final accepted blocker" Bug appeared in the same sync,

+         # but if one appeared first then the other appeared in a later

+         # sync, it wouldn't add the second

+         self.update_sync.sync_updates(self.test_release99)

+         bug2 = add_bug(2000, 'testbug2', self.test_milestone99beta)

+         self.update_sync.sync_updates(self.test_release99)

+         updates = Update.query.all()

+         assert len(updates) == 1

+         bugs = [bug for bug in updates[0].bugs]

+         assert len(bugs) == 2

+         assert bugs[0].bugid == bugs[1].bugid == bug1.bugid == bug2.bugid

+ 

      def test_sync_flatpak_updates(self):

          bug1 = add_bug(3100, 'testbug1', self.test_milestone99alpha)

          self.update_sync.sync_updates(self.test_release99)

The sync_updates code was written to only allow an update to
be associated with a single Bug for any given bug ID. But that's
not actually valid, because if a bug is for e.g. AcceptedFE for
Beta and AcceptedBlocker for Final, we encode this in the DB as
two Bugs with the same bug ID but different milestones. So we
need to allow the sync code to associate multiple Bugs for the
same bug ID with an update, if they have different milestones.

There was a loophole in the old code that meant it happened to
do the right thing if both the Bugs (representing both accepted
states) were picked up in the same sync - i.e. if the bug was
accepted as both Beta FE and Final blocker between two syncs.
But if the bug was accepted as one of the two, then a sync ran,
then it was accepted as the other and then another sync ran, the
second Bug (representing the second accepted state) was not
added. I think the loophole was an oversight and not intentional,
so I fixed it in this change, just in case we do somehow hit a
case where the query returns multiple Bugs with the same ID and
milestone (though there's no reason it ever should). The change
does make the code slightly less efficient because it now always
has to run the db query, but I don't think that's avoidable.

The symptom of this is, when you have an affected case, the bug
will appear on both the Beta and Final lists, but on one of the
two, the 'updatelabel' (the yellow 'testing' or green 'stable'
blob in the Updates column) will be missing, and in the Requests
tab, you will only get request text for one of the milestones
even though you should get it for both. For e.g. till today we
had bug #1872922 affected by this - it was both F33 Beta
AcceptedFE and F33 Final AcceptedBlocker, and the bug showed in
both bug lists, but though the update was in testing and queued
for stable, the updatelabel only appeared on the Final bug list
(not the Beta one) and the requests text for Beta didn't include
the update in the "stable push" list even though it should have
done, while the requests text for Final was correct.

It happened that the tooltip for the Updates column always
worked, though, which was the cause of the weird 'ghost update'
effect where there was no updatelabel but if you hovered over
the empty space, the update appeared in a tooltip.

Signed-off-by: Adam Williamson awilliam@redhat.com

Build succeeded.

Unfortunately Mohan was too efficient and pushed the real-world affected update stable before I could verify this change really fixed it. But I'm like 99% sure that was the problem and this is the solution. I did verify that the test I added fails in the way I'd expect if you revert the code change.

Seems to be in spirit with the recent changes in 25bac26 and the code looks good to me.

I see that if fullsync condition has been removed. Does it mean that effectively the --fullsync has lost meaning for syncing updates and we can remove it from the cmdline options and elsewhere, because we're now doing a full sync every time?

I'm not able to verify if the code does what it is supposed to, but I ran it locally and it seems to be syncing just fine. Also thanks for the unit test.

@kparal yeah, I think you're right, we could just ditch fullsync again at this point. There are other places where it's used but they all ultimately just pass it through here, I believe. I'll update the PR. I don't think the idea of a non-full sync was ever really a good one, because surely we always want to update the milestone information...

tagging @tflink since he introduced the full/not-full sync concept in f1085b0 , just in case he has any thoughts/notes.

oh, the "full sync" concept is still relevant in BugSync.update_active, so I'll keep it in the CLI for that.

rebased onto 2a2d501

3 years ago

rebased onto 13053e7

3 years ago

Build succeeded.

LGTM. You can wait for @tflink 's response, if you prefer, and feel free to merge the PR when you're ready.

Commit a5e11b1 fixes this pull-request

Pull-Request has been merged by kparal

3 years ago