PR#165: close pagure issues of inactive releases - fedora-qa/blockerbugs

		`@@ -0,0 +1,27 @@`
		`+ """Added discussions_closed column to releases`
		`+`
		`+ Revision ID: 4eac64cdecd3`
		`+ Revises: c88341bbda26`
		`+ Create Date: 2021-01-11 19:59:28.960876`
		`+`
		`+ """`
		`+`
		`+ # revision identifiers, used by Alembic.`
		`+ revision = '4eac64cdecd3'`
		`+ down_revision = 'c88341bbda26'`
		`+`
		`+ from alembic import op`
		`+ import sqlalchemy as sa`
		`+`
		`+`
		`+ def upgrade():`
		`+ # ### commands auto generated by Alembic - please adjust! ###`
		`+ op.add_column('release', sa.Column('discussions_closed', sa.Boolean(),`
		`+ server_default=sa.sql.expression.false(), nullable=False))`
		`+ # ### end Alembic commands ###`
		`+`
		`+`
		`+ def downgrade():`
		`+ # ### commands auto generated by Alembic - please adjust! ###`
		`+ op.drop_column('release', 'discussions_closed')`
		`+ # ### end Alembic commands ###`

blockerbugs/cli.py

file modified

+14 -1

		`@@ -220,11 +220,16 @@`

		`discussion_sync.recreate_discussion(bugid)`

		`+`
		`def update_discussion(args):`
		`ticket_id = int(args.ticket_id)`
		`pagure_bot.webhook_handler(ticket_id)`


		`+ def close_inactive_discussions(args):`
		`+ discussion_sync.close_discussions_inactive_releases(args.dryrun)`
		`+`
		`+`
		`def main():`
		`parser = ArgumentParser()`

		`@@ -302,12 +307,20 @@`
		`recreate_discussion_parser.set_defaults(func=recreate_discussion)`

		`update_discussion_parser = subparsers.add_parser('update-discussion',`
		`- help='Update discussion for a given ticket')`
		`+ help='Update discussion for a given ticket')`
		`update_discussion_parser.add_argument('ticket_id',`
		`metavar='<Ticket ID>',`
		`help='Ticket ID for which to update dicsussion')`
		`update_discussion_parser.set_defaults(func=update_discussion)`

		`+ close_inactive_discussions_parser = subparsers.add_parser('close-inactive-discussions',`
		`+ help="Close discussions of inactive releases (which haven't been processed yet)")`
		`+ close_inactive_discussions_parser.add_argument('--dryrun', action='store_true',`
		`+ default=False,`
		`+ help="Don't make any actual changes")`
		`+ close_inactive_discussions_parser.set_defaults(func=close_inactive_discussions)`
		`+`
		`+`
		`args = parser.parse_args()`

		`if not hasattr(args, 'func'):`

blockerbugs/models/release.py

file modified

+3 -1

		`@@ -27,11 +27,13 @@`
		`id = db.Column(db.Integer, primary_key=True)`
		`number = db.Column(db.Integer)`
		`active = db.Column(db.Boolean)`
		`+ discussions_closed = db.Column(db.Boolean)`
		`last_update_sync = db.Column(db.DateTime)`

		`- def __init__(self, number, active=True):`
		`+ def __init__(self, number, active=True, discussions_closed=False):`
		`self.number = number`
		`self.active = active`
		`+ self.discussions_closed = discussions_closed`
		`# initialize last_update_time so there's at least something there`
		`self.last_update_sync = datetime(1990, 1, 1, 1, 0, 0, 0)`

blockerbugs/util/discussion_sync.py

file modified

+55

		`@@ -4,6 +4,7 @@`
		`from blockerbugs.config import Config as bb_Config`
		`from blockerbugs.util import pagure_interface`
		`from blockerbugs.models.milestone import Milestone`
		`+ from blockerbugs.models.release import Release`
		`from blockerbugs.models.bug import Bug`


		`@@ -76,3 +77,57 @@`
		`for milestone in milestones:`
		`bugs = Bug.query.filter_by(bugid=bugid, milestone=milestone).all()`
		`create_discussions_links(milestone, bugs)`
		`+`
		`+`
		`+ def close_discussions_inactive_releases(dry_run=False):`
		`+ '''Close all Pagure discussion tickets for all inactive releases which are`
		+ not yet marked with ``Release.discussions_closed=True``.
		`+ '''`
		`+ app.logger.info('Closing discussion tickets in inactive releases...')`
		`+ inactive_releases = Release.query.filter_by(active=False, discussions_closed=False).all()`
		`+`
		`+ for inactive_release in inactive_releases:`
		`+ app.logger.info(f'Closing discussion tickets in release F{inactive_release.number}...')`
		`+ all_closed = True`
		`+ milestones = Milestone.query.filter_by(release=inactive_release).all()`
		`+ for milestone in milestones:`
		`+ bugs = Bug.query.filter_by(milestone=milestone)`
		`+ for bug in bugs:`
		`+ if not bug.discussion_link:`
		`+ app.logger.debug(f"Skipping bug {bug.bugid}, discussion doesn't exist")`
		`+ continue`
		`+`
		`+ try:`
		`+ # FIXME investigate storing issue_id directly in db instead of complete url`
		`+ issue_id = bug.discussion_link.split('/')[-1]`
		`+ app.logger.debug(f'Closing Pagure discussion #{issue_id} for bug {bug.bugid}')`
		`+`
		`+ status = pagure_interface.get_issue(issue_id)['status']`
		`+ if status == "Closed":`
		`+ app.logger.debug(f"Discussion issue #{issue_id} already closed, skipping")`
		`+ continue`
		`+`
		`+ comment = f"Release F{inactive_release.number} is no longer tracked by "\`
		`+ f"[BlockerBugs]({app.config['BLOCKERBUGS_URL']}), closing this "\`
		`+ "ticket."`
		`+`
		`+ if dry_run:`
		`+ app.logger.debug(f"[DRY RUN] Closing issue #{issue_id}")`
		`+ else:`
		`+ app.logger.debug(f"Closing issue #{issue_id}")`
		`+ pagure_interface.post_comment(issue_id, comment)`
		`+ pagure_interface.close_issue(issue_id)`
		`+ except pagure_interface.PagureAPIException as e:`
		`+ app.logger.error(f'Unable to close Pagure discussion #{issue_id} for bug '`
		`+ f'{bug.bugid}. Pagure error: {e}')`
		`+ all_closed = False`
		`+ continue`
		`+`
		`+ if all_closed and not dry_run:`
		`+ app.logger.debug("Setting dicussions_closed=True for release F%d" %`
		`+ inactive_release.number)`
		`+ inactive_release.discussions_closed = True`
		`+ db.session.add(inactive_release)`
		`+ db.session.commit()`
		`+`
		`+ app.logger.info('Closing discussion tickets in inactive releases done.')`

blockerbugs/util/pagure_interface.py

file modified

+23

		`@@ -85,6 +85,29 @@`
		`return issue_url`


		`+ def close_issue(issue_id):`
		`+ change_issue_status(issue_id, 'Closed')`
		`+`
		`+`
		`+ def change_issue_status(issue_id, status, close_status=None):`
		`+ change_issue_status_url = app.config['PAGURE_API'] + \`
		`+ app.config['PAGURE_REPO'] + '/issue/%s/status' % issue_id`
		`+ data = {`
		`+ 'status': status,`
		`+ }`
		`+ if close_status:`
		`+ data['close_status'] = close_status`
		`+`
		`+ resp = requests.post(change_issue_status_url,`
		`+ headers={`
		`+ 'Authorization': 'token %s' % app.config['PAGURE_REPO_TOKEN']`
		`+ },`
		`+ data=data)`
		`+ if resp.status_code != 200:`
		`+ raise PagureAPIException('Unable to change issue status, response code: %d, '`
		`+ 'response text:\n%s' % (resp.status_code, resp.text))`
		`+`
		`+`
		`def get_issue(issue_id):`
		`get_issue_url = app.config['PAGURE_API'] + app.config['PAGURE_REPO'] + '/issue/%s' % issue_id`
		`resp = requests.get(get_issue_url)`

testing/test_discussion_sync.py

file added

+111

		`@@ -0,0 +1,111 @@`
		`+ import mock`
		`+ from blockerbugs import db`
		`+ from testing.test_controllers import add_release, add_milestone, add_bug`
		`+ from blockerbugs.util import pagure_interface`
		`+ from blockerbugs.util import discussion_sync`
		`+ from blockerbugs.models.release import Release`
		`+`
		`+ stub_issue_open = {`
		`+ 'status': 'Open'`
		`+ }`
		`+`
		`+ stub_issue_closed = {`
		`+ 'status': 'Closed'`
		`+ }`
		`+`
		`+`
		`+ class TestInactiveDiscussions(object):`
		`+ def setup_method(self):`
		`+ db.session.rollback()`
		`+ db.drop_all()`
		`+ db.create_all()`
		`+`
		`+ self.release_a = add_release(134)`
		`+ self.release_b = add_release(133)`
		`+ self.release_b.active = False`
		`+`
		`+ self.milestone_a = add_milestone(self.release_a, 'final', 100, 101, '133-final', True)`
		`+ self.milestone_b = add_milestone(self.release_b, 'final', 200, 201, '134-final', True)`
		`+`
		`+ self.issue_id_a = "1"`
		`+ self.issue_id_b = "2"`
		`+`
		`+ bug1 = add_bug(9000, 'testbug1', self.milestone_a)`
		`+ bug1.discussion_link = "https://example.com/blockers/issue/%s" % self.issue_id_a`
		`+ bug2 = add_bug(9001, 'testbug2', self.milestone_b)`
		`+ bug2.discussion_link = "https://example.com/blockers/issue/%s" % self.issue_id_b`
		`+`
		`+ db.session.commit()`
		`+`
		`+ def teardown_method(self):`
		`+ db.session.rollback()`
		`+ db.drop_all()`
		`+`
		`+ def test_closing_commenting(self, monkeypatch):`
		`+ stub_get_issue_call = mock.MagicMock(return_value=stub_issue_open)`
		`+ monkeypatch.setattr(pagure_interface, 'get_issue', stub_get_issue_call)`
		`+`
		`+ stub_comment_call = mock.MagicMock()`
		`+ monkeypatch.setattr(pagure_interface, 'post_comment', stub_comment_call)`
		`+`
		`+ stub_close_call = mock.MagicMock()`
		`+ monkeypatch.setattr(pagure_interface, 'close_issue', stub_close_call)`
		`+`
		`+ discussion_sync.close_discussions_inactive_releases()`
		`+`
		`+ assert mock.call(self.issue_id_b) in stub_close_call.call_args_list`
		`+ assert mock.call(self.issue_id_a) not in stub_close_call.call_args_list`
		`+`
		`+ issues_commented = [c.args[0] for c in stub_comment_call.call_args_list]`
		`+ assert self.issue_id_b in issues_commented`
		`+ assert self.issue_id_a not in issues_commented`
		`+`
		`+ def test_issues_closed_no_comment(self, monkeypatch):`
		`+ stub_get_issue_call = mock.MagicMock(return_value=stub_issue_closed)`
		`+ monkeypatch.setattr(pagure_interface, 'get_issue', stub_get_issue_call)`
		`+`
		`+ stub_comment_call = mock.MagicMock()`
		`+ monkeypatch.setattr(pagure_interface, 'post_comment', stub_comment_call)`
		`+`
		`+ monkeypatch.setattr(pagure_interface, 'close_issue', mock.MagicMock())`
		`+`
		`+ discussion_sync.close_discussions_inactive_releases()`
		`+`
		`+ assert stub_comment_call.call_count == 0`
		`+`
		`+ def test_dry_run(self, monkeypatch):`
		`+ stub_get_issue_call = mock.MagicMock(return_value=stub_issue_open)`
		`+ monkeypatch.setattr(pagure_interface, 'get_issue', stub_get_issue_call)`
		`+`
		`+ stub_comment_call = mock.MagicMock()`
		`+ monkeypatch.setattr(pagure_interface, 'post_comment', stub_comment_call)`
		`+`
		`+ stub_close_call = mock.MagicMock()`
		`+ monkeypatch.setattr(pagure_interface, 'close_issue', stub_close_call)`
		`+`
		`+ stub_db = mock.MagicMock()`
		`+ monkeypatch.setattr(discussion_sync, 'db', stub_db)`
		`+`
		`+ discussion_sync.close_discussions_inactive_releases(dry_run=True)`
		`+`
		`+ assert stub_comment_call.call_count == 0`
		`+ assert stub_close_call.call_count == 0`
		`+ assert not stub_db.mock_calls`
		`+`
		`+ def test_release_discussions_closed(self, monkeypatch):`
		`+ stub_get_issue_call = mock.MagicMock(return_value=stub_issue_open)`
		`+ monkeypatch.setattr(pagure_interface, 'get_issue', stub_get_issue_call)`
		`+`
		`+ stub_comment_call = mock.MagicMock()`
		`+ monkeypatch.setattr(pagure_interface, 'post_comment', stub_comment_call)`
		`+`
		`+ stub_close_call = mock.MagicMock()`
		`+ monkeypatch.setattr(pagure_interface, 'close_issue', stub_close_call)`
		`+`
		`+ release = db.session.query(Release).filter_by(number=133).first()`
		`+ assert not release.discussions_closed`
		`+`
		`+ discussion_sync.close_discussions_inactive_releases()`
		`+`
		`+ release = db.session.query(Release).filter_by(number=133).first()`
		`+ assert release.discussions_closed`

lbrabec commented 3 years ago

#114

zuul commented 3 years ago

Build succeeded.

blockerbugs-pytest : SUCCESS in 1m 38s

kparal commented 3 years ago

After reading the code, especially close_discussions_inactive_releases() in discussion_sync.py, I realized that the current approach of going through all the tickets of all the bugs of all inactive releases has some... disadvantages. Currently we have 15 inactivate releases in the DB, and the number will grow. I didn't check, but we probably have a thousand or more bugs in the DB. We have around 180 discussion ticket links in the DB which refer to an inactive release, and the number will of course grow. Contacting Pagure about each ticket is possible and probably OK for manual execution, but doesn't seem OK for automation (e.g. if we ran this script daily), especially when looking a year ahead and more. Also the number of app.logger.debug() prints we do for each bug/discussion (not just the ones changing state) means probably several thousands lines in the blockerbugs log for each run. That also doesn't sound ideal. (Or perhaps I'm misunderstanding something?)

So, I see several possible approaches here:
1. Keep it as it is, but remove logging outputs for bugs/tickets which we don't do anything on (a bug without a discussion ticket, a ticket already closed), only log modification operations. Also never run this automated in the current state.
2. Adjust close_discussions_inactive_releases() to only run on a specified release, instead of all releases. That way we can say close_discussions(releasever='33') and limit the amount of API calls and log spam to only relevant bits. We can run this manually through a command, but also automate it (e.g. automatically run this when a release is set to inactive, through the webadmin or any other means).
3. Create a new property of the Release in DB, e.g. discussions_closed=bool. Set to False by default, and flip to to True, after all discussion tickets have been closed in this cleanup process. Next time the cleanup is run, skip all releases which have discussions_closed=True. Very effective and self-maintaining, automation-friendly, but requires a DB update.
4. Try to figure out when the Release was closed, and only run on recently-closed releases by default. We can't exactly say when it was closed without modifying the DB, but we do have a Release.last_update_sync property. And we don't do syncs on inactive releases. So while it's not completely clean, it's almost exactly what we're looking for. We can keep the code as it is, but skip all Releases which have last_update_sync older than e.g. 6 months. (We should still adjust the logging a bit, in case we wanted to run this e.g. every day).

I like 3) and 4) the most, I think. Thoughts?

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

I see this as a too short name. Inactive what? We have releases, milestones, bugs, updates, discussions. It would be less confusing to uses a longer name.

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

Also here the parser name "close inactive releases" is somewhat confusing, because this is not about closing releases, but closing discussions.

kparal commented on line 15 of blockerbugs/util/discussion_sync.py 3 years ago

This is funny. I don't remember any API which would require you to override an argument to actually do something :-) I'm not saying we can't do it, but it's... interesting. Especially when our own command line performs operation by default, and you can optionally change it to a dry run, but our API works in reverse :-) I'd personally default to dry_run=False here.

kparal commented on line 20 of blockerbugs/util/discussion_sync.py 3 years ago

I'm not sure what the comment means here. I guess it should be "became"? Is that somehow related to my overall comment that we could only run on recently-closed releases? This comment should be clearer, thanks.

kparal commented on line 29 of blockerbugs/util/discussion_sync.py 3 years ago

Honestly I think this is OK for the moment, no hard feelings.

kparal commented on line 16 of blockerbugs/util/discussion_sync.py 3 years ago

There should be some "starting" log output here, similar to what I suggested in https://pagure.io/fedora-qa/blockerbugs/issue/114#comment-698788 . If there's no clear "start operation" log output first, it might not be clear why we see the following log output like "discussion does not exist, nothing to close". Also, a static printout helps us grep the logs when debugging.

kparal commented on line 23 of blockerbugs/util/discussion_sync.py 3 years ago

I know that my "80 characters on a line" world-view is becoming as outdated as editing code in vim, but could we please at least agree on honoring the 100 column limit? :-)

kparal commented 3 years ago

If I can have an additional wish, that would be at least a few basic unit tests. Thanks!

lbrabec commented 3 years ago

So, I see several possible approaches here:
...
I like 3) and 4) the most, I think. Thoughts?

I'd go with 3). The DB change will be quite small. Any objections @jskladan ?

This is funny. I don't remember any API which would require you to override an argument to actually do something :-) I'm not saying we can't do it, but it's... interesting. Especially when our own command line performs operation by default, and you can optionally change it to a dry run, but our API works in reverse :-) I'd personally default to dry_run=False here.

Good catch, thanks. It's a relic from testing...

I'm not sure what the comment means here. I guess it should be "became"? Is that somehow related to my overall comment that we could only run on recently-closed releases? This comment should be clearer, thanks.

dtto, it was a note for me, I'll remove the comment.