PR#129: Create one discussion per bug and release - fedora-qa/blockerbugs

fedora-qa / blockerbugs

#129 Create one discussion per bug and release

Merged 3 years ago by kparal. Opened 3 years ago by lbrabec.

fix/linkperrelease into develop

Create one discussion per bug and release

Lukas Brabec • 3 years ago

32e0fc9

blockerbugs/cli.py

file modified

+40 -26

		`@@ -184,6 +184,33 @@`
		`or bug.accepted_fe)`


		`+ def _link_key(milestone, bug):`
		`+ return "%d_%d" % (milestone.release.number, bug.bugid)`
		`+`
		`+`
		`+ def create_discussions_links(milestone, bugs, links_to_reuse={}):`
		`+ for bug in bugs:`
		`+ if not needs_discussion(bug):`
		`+ app.logger.debug('Skipping bug %d, no discussion needed' % bug.bugid)`
		`+ continue`
		`+`
		`+ link = links_to_reuse.get(_link_key(milestone, bug))`
		`+ if link:`
		`+ app.logger.debug('Reusing discussion link for bug %d' % bug.bugid)`
		`+ else:`
		`+ app.logger.debug('Creating Pagure discussion for bug %d' % bug.bugid)`
		`+ try:`
		`+ link = pagure_interface.create_bug_discussion(bug)`
		`+ links_to_reuse[_link_key(milestone, bug)] = link`
		`+ except pagure_interface.PagureAPIException as e:`
		`+ app.logger.error('Unable to create Pagure discussion for bug %s. '`
		`+ 'Pagure error: %s' % (bug.bugid, e))`
		`+ continue`
		`+`
		`+ bug.discussion_link = link`
		`+ db.session.add(bug)`
		`+ db.session.commit()`
		`+`
		`def sync_discussions(args):`
		`if app.config["PAGURE_REPO_TOKEN"] in (bb_Config.PAGURE_REPO_TOKEN, ""):`
		`# skip this if the API token is set to the placeholder value`
		`@@ -191,28 +218,19 @@`
		`app.logger.debug('Not syncing discussions, because PAGURE_REPO_TOKEN '`
		`'is not configured.')`
		`return`
		`+`
		`+ links = {}`
		`+`
		`active_milestones = Milestone.query.filter_by(active=True).all()`

		`for milestone in active_milestones:`
		`- bugs = Bug.query.filter_by(milestone=milestone, active=True)`
		`- for bug in bugs:`
		`- if bug.discussion_link:`
		`- app.logger.debug('Skipping bug %d, discussion already exists' % bug.bugid)`
		`- continue`
		`- if not needs_discussion(bug):`
		`- app.logger.debug('Skipping bug %d, no discussion needed' % bug.bugid)`
		`- continue`
		`+ bugs = Bug.query.filter_by(milestone=milestone, active=True).all()`
		`+ for bug in (b for b in bugs if b.discussion_link):`
		`+ links.setdefault(_link_key(milestone, bug), bug.discussion_link)`

		`- app.logger.debug('Creating Pagure discussion for bug %d' % bug.bugid)`
		`- try:`
		`- bug.discussion_link = pagure_interface.create_bug_discussion(bug)`
		`- except pagure_interface.PagureAPIException as e:`
		`- app.logger.error('Unable to create Pagure discussion for bug %s. '`
		`- 'Pagure error: %s' % (bug.bugid, e))`
		`- continue`
		`-`
		`- db.session.add(bug)`
		`- db.session.commit()`
		`+ for milestone in active_milestones:`
		`+ bugs = Bug.query.filter_by(milestone=milestone, active=True, discussion_link=None).all()`
		`+ create_discussions_links(milestone, bugs, links_to_reuse=links)`


		`def recreate_discussion(args):`
		`@@ -223,14 +241,10 @@`
		`app.logger.error('No bug %d in databse.' % bugid)`
		`sys.exit(1)`

		`- try:`
		`- bug.discussion_link = pagure_interface.create_bug_discussion(bug)`
		`- except pagure_interface.PagureAPIException as e:`
		`- app.logger.error('Unable to create Pagure discussion for bug %s. '`
		`- 'Pagure error: %s' % (bug.bugid, e))`
		`-`
		`- db.session.add(bug)`
		`- db.session.commit()`
		`+ milestones = Milestone.query.all()`
		`+ for milestone in milestones:`
		`+ bugs = Bug.query.filter_by(bugid=bugid, milestone=milestone).all()`
		`+ create_discussions_links(milestone, bugs)`


		`def sync_bugs(args):`

lbrabec commented 3 years ago

Fix for #127

kparal commented 3 years ago

Looking at the code, there should clearly exist util/discussion_sync.py instead of having all this code in cli.py. But let's move it in a separate patch, just noting.

jskladan commented on line 62 of blockerbugs/cli.py 3 years ago

This could be a generator-expression instead of list-comprehension. Absolutely a nitpick thought

kparal commented on line 47 of blockerbugs/cli.py 3 years ago

This condition got removed, but I don't see it added anywhere. Wouldn't it mean that all bugs will get processed every time, and the same value will be reassigned to them every time (and a debug log will get printed every time)?

jskladan commented on line 75 of blockerbugs/cli.py 3 years ago

I'd consider keeping the old code with first() here. since you are not re-using the bugs value anyway, it's no difference if you only get one, or more of the results, and first() can be significantly faster than all().

jskladan commented 3 years ago

LGTM otherwise.

lbrabec commented 3 years ago

This condition got removed, but I don't see it added anywhere. Wouldn't it mean that all bugs will get processed every time, and the same value will be reassigned to them every time (and a debug log will get printed every time)?

It's two pass process. The first pass gathers all the links from bugs that have link, second pass iterates over bugs without link. The condition is not needed, if I'm not missing something.

1 new commit added

use first() to check if bug is in db

3 years ago

kparal commented 3 years ago

It's two pass process. The first pass gathers all the links from bugs that have link, second pass iterates over bugs without link. The condition is not needed, if I'm not missing something.

I missed discussion_link=None in filter. Yes, it should work as you say,

kparal commented 3 years ago

If only we had staging infra :-/ I couldn't test this because I don't have Pagure deployed locally at the moment. But the code looks OK.

1 new commit added

use generator-expression instead of list-comprehension

3 years ago

kparal commented 3 years ago

I created a test project on Pagure and I can confirm this branch creates just a single ticket for a bug marked against two different trackers. It also created other tickets just fine. So it seems to be working.

Note: I see all messages produced by app.logger triplicated, which is the approach this PR uses. Other files using logging.getLogger() produce just a single line. That's probably not related, just mentioning something we need to fix.

kparal commented 3 years ago

Note: I see all messages produced by app.logger triplicated, which is the approach this PR uses. Other files using logging.getLogger() produce just a single line. That's probably not related, just mentioning something we need to fix.

Fixed in #133

kparal commented 3 years ago

Can we push this? Does anyone have any more concerns?

kparal commented 3 years ago

(Let's squash this before merging)

jskladan commented 3 years ago

Fine with me, don't forget to rebase on current devel though ;)

rebased onto 32e0fc9

3 years ago

lbrabec commented 3 years ago

Squashed and rebased.

jskladan commented 3 years ago