PR#3296: comments for grant/revoke permissions - koji

koji

#3296 comments for grant/revoke permissions

Opened 2 years ago by tkopecek. Modified 2 years ago

tkopecek/koji issue3295 into master

comments for grant/revoke permissions

Tomas Kopecek • 2 years ago

a0112b8

cli/koji_cli/commands.py

file modified

+12 -1

		`@@ -2445,6 +2445,8 @@`
		`help="Create this permission if the permission does not exist")`
		`parser.add_option("--description",`
		`help="Add description about new permission")`
		`+ parser.add_option("--comment",`
		`+ help="Additional comment which will be seen in the history.")`
		`(options, args) = parser.parse_args(args)`
		`if len(args) < 2:`
		`parser.error("Please specify a permission and at least one user")`
		`@@ -2462,6 +2464,8 @@`
		`kwargs['create'] = True`
		`if options.description:`
		`kwargs['description'] = options.description`
		`+ if options.comment:`
		`+ kwargs['label'] = options.comment`
		`if options.description and not options.new:`
		`parser.error("Option new must be specified with option description.")`
		`for user in users:`
		`@@ -2472,6 +2476,8 @@`
		`"[admin] Revoke a permission from a user"`
		`usage = "usage: %prog revoke-permission <permission> <user> [<user> ...]"`
		`parser = OptionParser(usage=get_usage_str(usage))`
		`+ parser.add_option("--comment",`
		`+ help="Additional comment which will be seen in the history.")`
		`(options, args) = parser.parse_args(args)`
		`if len(args) < 2:`
		`parser.error("Please specify a permission and at least one user")`
		`@@ -2484,8 +2490,11 @@`
		`if user is None:`
		`parser.error("No such user: %s" % n)`
		`users.append(user)`
		`+ kwargs = {}`
		`+ if options.comment:`
		`+ kwargs['label'] = options.comment`
		`for user in users:`
		`- session.revokePermission(user['name'], perm)`
		`+ session.revokePermission(user['name'], perm, **kwargs)`


		`def handle_edit_permission(goptions, session, args):`
		`@@ -4670,6 +4679,8 @@`
		`parts.insert(1, "(eid %i)" % event_id)`
		`if who:`
		`parts.append(who % x)`
		`+ if x['create_event_label']:`
		`+ parts.append("(%s)" % x['create_event_label'])`
		`if create and x['active']:`
		`parts.append("[still active]")`
		`print(' '.join(parts))`

hub/kojihub.py

file modified

+42 -10

		`@@ -7992,12 +7992,16 @@`
		`"LEFT OUTER JOIN events AS ev2 ON ev2.id = revoke_event",`
		`"users AS creator ON creator.id = creator_id",`
		`"LEFT OUTER JOIN users AS revoker ON revoker.id = revoker_id",`
		`+ "LEFT OUTER JOIN event_labels AS ev1_labels ON ev1_labels.event_id = create_event",`
		`+ "LEFT OUTER JOIN event_labels AS ev2_labels ON ev2_labels.event_id = revoke_event",`
		`]`
		`common_joined_fields = {`
		`'creator.name': 'creator_name',`
		`'revoker.name': 'revoker_name',`
		`'EXTRACT(EPOCH FROM ev1.time) AS create_ts': 'create_ts',`
		`'EXTRACT(EPOCH FROM ev2.time) AS revoke_ts': 'revoke_ts',`
		`+ 'ev1_labels.label': 'create_event_label',`
		`+ 'ev2_labels.label': 'revoke_event_label',`
		`}`
		`table_fields = {`
		`'user_perms': ['user_id', 'perm_id'],`
		`@@ -9007,7 +9011,7 @@`
		`raise koji.AuthError("Content generator access required (%s)" % cg['name'])`


		`- def get_event():`
		`+ def get_event(label=None):`
		`"""Get an event id for this transaction`

		`We cache the result in context, so subsequent calls in the same transaction will`
		`@@ -9020,6 +9024,11 @@`
		`return context.event_id`
		`event_id = _singleValue("SELECT get_event()")`
		`context.event_id = event_id`
		`+ if label:`
		`+ # there could be more than one label per event`
		`+ insert = InsertProcessor('event_labels')`
		`+ insert.set(event_id=event_id, label=label)`
		`+ insert.execute()`
		`return event_id`


		`@@ -9195,9 +9204,15 @@`
		`"""Set rawdata via keyword args"""`
		`self.rawdata.update(kwargs)`

		`- def make_create(self, event_id=None, user_id=None):`
		`+ def make_create(self, event_id=None, user_id=None, label=None):`
		`if event_id is None:`
		`- event_id = get_event()`
		`+ event_id = get_event(label)`
		`+ elif label:`
		`+ update = UpdateProcessor('event_labels',`
		`+ clauses=['id=%(event_id)'],`
		`+ values={'event_id': event_id})`
		`+ update.set(label=label)`
		`+ update.execute()`
		`if user_id is None:`
		`context.session.assertLogin()`
		`user_id = context.session.user_id`
		`@@ -9284,10 +9299,16 @@`
		`"""Set rawdata via keyword args"""`
		`self.rawdata.update(kwargs)`

		`- def make_revoke(self, event_id=None, user_id=None):`
		`+ def make_revoke(self, event_id=None, user_id=None, label=None):`
		`"""Add standard revoke options to the update"""`
		`if event_id is None:`
		`- event_id = get_event()`
		`+ event_id = get_event(label=label)`
		`+ elif label:`
		`+ update = UpdateProcessor('event_labels',`
		`+ clauses=['id=%(event_id)'],`
		`+ values={'event_id': event_id})`
		`+ update.set(label=label)`
		`+ update.execute()`
		`if user_id is None:`
		`context.session.assertLogin()`
		`user_id = context.session.user_id`
		`@@ -12437,8 +12458,19 @@`
		`getUser = staticmethod(get_user)`
		`editUser = staticmethod(edit_user)`

		`- def grantPermission(self, userinfo, permission, create=False, description=None):`
		`- """Grant a permission to a user"""`
		`+ def grantPermission(self, userinfo, permission, create=False, description=None, label=None):`
		`+ """Grant a permission to a user`
		`+`
		`+ :param userInfo: a str (Kerberos principal or name) or an int (user id)`
		`+ or a dict:`
		`+ - id: User's ID`
		`+ - name: User's name`
		`+ - krb_principal: Kerberos principal`
		`+ :param str permission: Permission name`
		`+ :param bool create: Create permission if it doesn't exist`
		`+ :param str description: In case of creating new permission, description can be specified`
		`+ :param str label: Reason/comment for grant operation`
		`+ """`
		`context.session.assertPerm('admin')`
		`if create:`
		`verify_name_internal(permission)`
		`@@ -12457,10 +12489,10 @@`
		`(userinfo, perm['name']))`
		`insert = InsertProcessor('user_perms')`
		`insert.set(user_id=user_id, perm_id=perm_id)`
		`- insert.make_create()`
		`+ insert.make_create(label=label)`
		`insert.execute()`

		`- def revokePermission(self, userinfo, permission):`
		`+ def revokePermission(self, userinfo, permission, label=None):`
		`"""Revoke a permission from a user"""`
		`context.session.assertPerm('admin')`
		`user_id = get_user(userinfo, strict=True)['id']`
		`@@ -12471,7 +12503,7 @@`
		`(userinfo, perm['name']))`
		`update = UpdateProcessor('user_perms', values=locals(),`
		`clauses=["user_id = %(user_id)i", "perm_id = %(perm_id)i"])`
		`- update.make_revoke()`
		`+ update.make_revoke(label=label)`
		`update.execute()`

		`def editPermission(self, permission, description):`

tests/test_cli/test_list_history.py

file modified

+66

		`@@ -232,12 +232,14 @@`
		`'channel_id': 1,`
		`'channels.name': 'default',`
		`'create_event': 3,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612355089.887727,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'host.name': 'kojibuilder',`
		`'host_id': 1,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None}]`
		`@@ -260,12 +262,14 @@`
		`'channel_id': 1,`
		`'channels.name': 'default',`
		`'create_event': 3,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612355089.887727,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'host.name': 'kojibuilder',`
		`'host_id': 1,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None}]`
		`@@ -285,12 +289,14 @@`
		`'channel_id': 1,`
		`'channels.name': 'default',`
		`'create_event': 3,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612355089.887727,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'host.name': 'kojibuilder',`
		`'host_id': 1,`
		`'revoke_event': 8,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': 1612355099.887727,`
		`'revoker_id': 1,`
		`'revoker_name': 'kojiadmin'}]`
		`@@ -328,6 +334,7 @@`
		`'capacity': 2.0,`
		`'comment': None,`
		`'create_event': 2,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612355089.886359,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -336,6 +343,7 @@`
		`'host.name': 'kojibuilder',`
		`'host_id': 1,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None}]`
		`@@ -357,6 +365,7 @@`
		`'capacity': 2.0,`
		`'comment': None,`
		`'create_event': 2,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612355089.886359,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -365,6 +374,7 @@`
		`'host.name': 'kojibuilder',`
		`'host_id': 1,`
		`'revoke_event': 3,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': 1612355099.886359,`
		`'revoker_id': 1,`
		`'revoker_name': 'kojiadmin'}]`
		`@@ -403,6 +413,7 @@`
		`'build.state': 1,`
		`'build_id': 6,`
		`'create_event': 585,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613744957.14465,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -410,6 +421,7 @@`
		`'name': 'test-build',`
		`'release': '11',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -433,6 +445,7 @@`
		`'build.state': 1,`
		`'build_id': 6,`
		`'create_event': 585,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613744957.14465,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -440,6 +453,7 @@`
		`'name': 'test-build',`
		`'release': '11',`
		`'revoke_event': 590,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': 1613744967.14465,`
		`'revoker_id': 1,`
		`'revoker_name': 'kojiadmin',`
		`@@ -483,6 +497,7 @@`
		`'build.state': 1,`
		`'build_id': 4,`
		`'create_event': 424,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613736474.42776,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -490,6 +505,7 @@`
		`'name': 'pkg-name',`
		`'release': '11',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -499,6 +515,7 @@`
		`'tag_package_owners': [`
		`{'active': True,`
		`'create_event': 418,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613736220.79199,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -507,6 +524,7 @@`
		`'package.name': 'pkg-name',`
		`'package_id': 4,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -516,6 +534,7 @@`
		`{'active': True,`
		`'blocked': False,`
		`'create_event': 418,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613736220.79199,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -523,6 +542,7 @@`
		`'package.name': 'pkg-name',`
		`'package_id': 4,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -560,6 +580,7 @@`
		`{'active': True,`
		`'arches': 'x86_64',`
		`'create_event': 6,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612872591.313584,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -569,6 +590,7 @@`
		`'perm_id': None,`
		`'permission.name': None,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -577,11 +599,13 @@`
		`'tag_extra': [`
		`{'active': True,`
		`'create_event': 6,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612872591.313584,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'key': 'mock.package_manager',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -620,12 +644,14 @@`
		`'build_target.name': 'build-target-test-tag',`
		`'build_target_id': 1,`
		`'create_event': 8,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612872656.231499,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'dest_tag': 3,`
		`'dest_tag.name': 'destination-test-tag',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None}],`
		`@@ -636,12 +662,14 @@`
		`'channel_id': 1,`
		`'channels.name': 'default',`
		`'create_event': 3,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612355089.887727,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'host.name': 'kojibuilder',`
		`'host_id': 1,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None}],`
		`@@ -653,6 +681,7 @@`
		`'capacity': 2.0,`
		`'comment': None,`
		`'create_event': 2,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612355089.886359,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -661,6 +690,7 @@`
		`'host.name': 'kojibuilder',`
		`'host_id': 1,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None}],`
		`@@ -670,6 +700,7 @@`
		`'active': True,`
		`'arches': '',`
		`'create_event': 5,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612871243.593475,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -679,6 +710,7 @@`
		`'perm_id': None,`
		`'permission.name': None,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -689,11 +721,13 @@`
		`'_revoked_by': None,`
		`'active': True,`
		`'create_event': 6,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612872591.313584,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'key': 'mock.package_manager',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -705,6 +739,7 @@`
		`'_revoked_by': True,`
		`'active': None,`
		`'create_event': 16,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613545870.370125,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -716,6 +751,7 @@`
		`'pkg_filter': '',`
		`'priority': 1,`
		`'revoke_event': 31,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': 1613545887.513565,`
		`'revoker_id': 1,`
		`'revoker_name': 'kojiadmin',`
		`@@ -726,6 +762,7 @@`
		`'_revoked_by': None,`
		`'active': True,`
		`'create_event': 9,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612872778.934647,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -734,6 +771,7 @@`
		`'package.name': 'koji',`
		`'package_id': 1,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -745,6 +783,7 @@`
		`'active': True,`
		`'blocked': False,`
		`'create_event': 9,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612872778.934647,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -752,6 +791,7 @@`
		`'package.name': 'koji',`
		`'package_id': 1,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -762,12 +802,14 @@`
		`'_revoked_by': None,`
		`'active': True,`
		`'create_event': 1,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612355089.882428,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'perm_id': 1,`
		`'permission.name': 'admin',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -815,6 +857,7 @@`
		`'tag_package_owners': [`
		`{'active': True,`
		`'create_event': 9,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613730799.934647,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -823,6 +866,7 @@`
		`'package.name': 'koji',`
		`'package_id': 1,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -832,12 +876,14 @@`
		`'user_perms': [`
		`{'active': True,`
		`'create_event': 1,`
		`+ 'create_event_label': None,`
		`'create_ts': 1612355089.882428,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'perm_id': 1,`
		`'permission.name': 'admin',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -845,12 +891,14 @@`
		`'user_id': 1},`
		`{'active': None,`
		`'create_event': 6,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613730777.437744,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'perm_id': 4,`
		`'permission.name': 'dist-repo',`
		`'revoke_event': 7,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': 1613730790.797031,`
		`'revoker_id': 1,`
		`'revoker_name': 'kojiadmin',`
		`@@ -889,12 +937,14 @@`
		`'user_perms': [`
		`{'active': True,`
		`'create_event': 20,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613730777.437744,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'perm_id': 4,`
		`'permission.name': 'dist-repo',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -915,12 +965,14 @@`
		`'user_perms': [`
		`{'active': None,`
		`'create_event': 25,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613730787.437744,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'perm_id': 4,`
		`'permission.name': 'dist-repo',`
		`'revoke_event': 27,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': 1613730797.797031,`
		`'revoker_id': 1,`
		`'revoker_name': 'kojiadmin',`
		`@@ -955,10 +1007,12 @@`
		`'cg_id': 1,`
		`'content_generator.name': 'test-cg',`
		`'create_event': 425,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613736494.11504,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -979,10 +1033,12 @@`
		`'cg_id': 2,`
		`'content_generator.name': 'test-cg',`
		`'create_event': 430,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613736499.11504,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'revoke_event': 440,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': 1613736510.11504,`
		`'revoker_id': 1,`
		`'revoker_name': 'kojiadmin',`
		`@@ -1010,12 +1066,14 @@`
		`'external_repo_config': [`
		`{'active': True,`
		`'create_event': 342,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613736061.68861,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'external_repo.name': 'external-repo-test',`
		`'external_repo_id': 5,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -1024,6 +1082,7 @@`
		`'tag_external_repos': [`
		`{'active': True,`
		`'create_event': 343,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613736061.72882,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`@@ -1032,6 +1091,7 @@`
		`'merge_mode': 'koji',`
		`'priority': 5,`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`
		`@@ -1073,12 +1133,14 @@`
		`'build_target.name': 'test-build-target',`
		`'build_target_id': 5,`
		`'create_event': 420,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613736230.7615,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'dest_tag': 11,`
		`'dest_tag.name': 'destination-test-tag',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None}]`
		`@@ -1101,12 +1163,14 @@`
		`'build_target.name': 'test-build-target',`
		`'build_target_id': 4,`
		`'create_event': 417,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613736220.7615,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'dest_tag': 10,`
		`'dest_tag.name': 'destination-test-tag',`
		`'revoke_event': 420,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': 1613736225.7615,`
		`'revoker_id': 1,`
		`'revoker_name': 'kojiadmin'}]`
		`@@ -1136,11 +1200,13 @@`
		`'tag_extra': [`
		`{'active': True,`
		`'create_event': 586,`
		`+ 'create_event_label': None,`
		`'create_ts': 1613744977.02986,`
		`'creator_id': 1,`
		`'creator_name': 'kojiadmin',`
		`'key': 'extra-key-test',`
		`'revoke_event': None,`
		`+ 'revoke_event_label': None,`
		`'revoke_ts': None,`
		`'revoker_id': None,`
		`'revoker_name': None,`

tkopecek commented 2 years ago

mikem commented 2 years ago

My first thought is that this is a proverbial "can of worms". This patch only adds comments for two operations, but hints at comments for all events.

I'm not sure event_labels is the right thing. We've never used it, and I don't think we need to be constrained by it. This is a significant feature to add and it's worth getting right. In particular I'm not sure about:

the 255 character limit
the multiple labels per event
the term "label"

I have a couple notes about the particulars here, but frankly they might not matter if we need to take a different approach.

print_histline seems to handle create_event_label but not revoke_event_label
how this affects event caching in get_event (what if get_event is called with different labels in the same transaction)
what if we pass both an event_id and a label to make_create/make_revoke?

mikem commented 2 years ago

I think event labels were envisioned to be more like short names for events (analogous to tags in source control systems), hence the limited character length. The term comment suggests a bit more room for freeform text. Note that labels are unique in the db. Admins would not be able to make the same comment twice.

    label VARCHAR(255) UNIQUE NOT NULL

Do we have a sense of what limits we might want, if any, on the length of a comment? Perhaps it might work like a git commit message where the first line is considered a subject for short display purposes, but long messages are allowed.

tkopecek commented 2 years ago

I was wondering also if this is the right approach. Do we want (in future) really comments to all events? In such case it would be nice to have generic API (addEventComment/editEventComment/deleteEventComment, getEvent(comment=True)) with some permissions - e.g. add can anybody, edit/delete only owner/admin? Separate calls would solve those issues with reusing same event_id in make_create/make_revoke, etc. It will be simply separate function called after successful event creation/selection.

I would allow multiple notes per event (as is in the PR) than to modifying the same one. Of course other possibility is simply to modify existing comment (another addEventNote would simply add lines to existing one). But I think it is more counter-intuitive behaviour (e.g. when searching for some concrete comment with specific first line (when/if API will be available).

New table could look like

CREATE TABLE event_comments (
   event_id INTEGER NOT NULL REFERENCES events(id),
   author NOT NULL REFERENCES users(id),
   created_ts TIMESTAMPTZ NOT NULL DEFAULT NOW(),
   title VARCHAR(255) NOT NULL,
   description TEXT,
) WITHOUT OIDS

Not sure about splitting title/description or taking first line (or simply something like SELECT LEFT(description, 80) could be enough?)
Default ordering by id should be sufficient to get reasonable order for multiple comments.