#2495 blocking inherited extra
Merged 9 months ago by tkopecek. Opened 9 months ago by tkopecek.
tkopecek/koji issue2283  into  master

file modified
+4 -2
@@ -4987,6 +4987,8 @@ 

                        help=_("Set tag extra option"))

      parser.add_option("-r", "--remove-extra", action="append", default=[], metavar="key",

                        help=_("Remove tag extra option"))

+     parser.add_option("-b", "--block-extra", action="append", default=[], metavar="key",

+                       help=_("Block inherited tag extra option"))

      (options, args) = parser.parse_args(args)

      if len(args) != 1:

          parser.error(_("Please specify a name for the tag"))
@@ -5020,8 +5022,8 @@ 

              value = arg_filter(value)

              extra[key] = value

          opts['extra'] = extra

-     if options.remove_extra:

-         opts['remove_extra'] = options.remove_extra

+     opts['remove_extra'] = options.remove_extra

+     opts['block_extra'] = options.block_extra

      # XXX change callname

      session.editTag2(tag, **opts)

  

@@ -0,0 +1,9 @@ 

+ -- upgrade script to migrate the Koji database schema

+ -- from version 1.23 to 1.24

+ 

+ 

+ BEGIN;

+ 

+ ALTER TABLE tag_extra ALTER COLUMN value DROP NOT NULL;

+ 

+ COMMIT;

file modified
+1 -1
@@ -394,7 +394,7 @@ 

  CREATE TABLE tag_extra (

  	tag_id INTEGER NOT NULL REFERENCES tag(id),

  	key TEXT NOT NULL,

- 	value TEXT NOT NULL,  -- TODO - move this to jsonb when we can

+ 	value TEXT,

  -- versioned - see desc above

  	create_event INTEGER NOT NULL REFERENCES events(id) DEFAULT get_event(),

  	revoke_event INTEGER REFERENCES events(id),

file modified
+51 -16
@@ -3266,7 +3266,7 @@ 

      return tag_id

  

  

- def get_tag(tagInfo, strict=False, event=None):

+ def get_tag(tagInfo, strict=False, event=None, blocked=False):

      """Get tag information based on the tagInfo.  tagInfo may be either

      a string (the tag name) or an int (the tag ID).

      Returns a map containing the following keys:
@@ -3318,21 +3318,28 @@ 

          if strict:

              raise koji.GenericError("Invalid tagInfo: %r" % tagInfo)

          return None

-     result['extra'] = get_tag_extra(result, event)

+     result['extra'] = get_tag_extra(result, event, blocked=blocked)

      return result

  

  

- def get_tag_extra(tagInfo, event=None):

+ def get_tag_extra(tagInfo, event=None, blocked=False):

      """ Get tag extra info (no inheritance) """

      tables = ['tag_extra']

-     fields = ['key', 'value']

+     fields = ['key', 'value', 'CASE WHEN value IS NULL THEN TRUE ELSE FALSE END']

+     aliases = ['key', 'value', 'blocked']

      clauses = [eventCondition(event, table='tag_extra'), "tag_id = %(id)i"]

+     if not blocked:

+         clauses.append("value IS NOT NULL")

      query = QueryProcessor(columns=fields, tables=tables, clauses=clauses, values=tagInfo,

-                            opts={'asList': True})

+                            aliases=aliases)

      result = {}

-     for key, value in query.execute():

-         value = parse_json(value, errstr="Invalid tag extra data: %s" % key)

-         result[key] = value

+     for h in query.execute():

+         if h['value'] is not None:

+             h['value'] = parse_json(h['value'], errstr="Invalid tag extra data: %s" % h['key'])

+         if blocked:

+             result[h['key']] = (h['blocked'], h['value'])

+         else:

+             result[h['key']] = h['value']

      return result

  

  
@@ -3357,6 +3364,7 @@ 

                                     the Maven repo.

      :param dict extra: add or update extra tag parameters.

      :param list remove_extra: remove extra tag parameters.

+     :param list block_extra: block inherited extra tag parameters.

      """

  

      context.session.assertPerm('tag')
@@ -3425,19 +3433,23 @@ 

  

      # handle extra data

      if 'extra' in kwargs:

+         removed = set(kwargs.get('block_extra', [])) | set(kwargs.get('remove_extra', []))

          # check whether one key is both in extra and remove_extra

-         if 'remove_extra' in kwargs:

-             for removed in kwargs['remove_extra']:

-                 if removed in kwargs['extra']:

-                     raise koji.GenericError("Can not both add/update and remove tag-extra: '%s'" %

-                                             removed)

+         conflicts = removed.intersection(set(kwargs['extra']))

+         if conflicts:

+             raise koji.GenericError("Can not both add/update and remove tag-extra: '%s'" %

+                                     conflicts.pop())

          for key in kwargs['extra']:

              value = kwargs['extra'][key]

              if key not in tag['extra'] or tag['extra'][key] != value:

+                 if value is None:

+                     value = 'null'

+                 else:

+                     value = json.dumps(value)

                  data = {

                      'tag_id': tag['id'],

                      'key': key,

-                     'value': json.dumps(kwargs['extra'][key]),

+                     'value': value,

                  }

                  # revoke old entry, if any

                  update = UpdateProcessor('tag_extra', values=data, clauses=['tag_id = %(tag_id)i',
@@ -3449,6 +3461,23 @@ 

                  insert.make_create()

                  insert.execute()

  

+     if 'block_extra' in kwargs:

+         for key in kwargs['block_extra']:

+             data = {

+                 'tag_id': tag['id'],

+                 'key': key,

+                 'value': None,

+             }

+             # revoke old entry, if any

+             update = UpdateProcessor('tag_extra', values=data, clauses=['tag_id = %(tag_id)i',

+                                                                         'key=%(key)s'])

+             update.make_revoke()

+             update.execute()

+             # add new entry

+             insert = InsertProcessor('tag_extra', data=data)

+             insert.make_create()

+             insert.execute()

+ 

      # handle remove_extra data

      if 'remove_extra' in kwargs:

          ne = [e for e in kwargs['remove_extra'] if e not in tag['extra']]
@@ -11916,18 +11945,24 @@ 

  

      def getBuildConfig(self, tag, event=None):

          """Return build configuration associated with a tag"""

-         taginfo = get_tag(tag, strict=True, event=event)

+         taginfo = get_tag(tag, strict=True, event=event, blocked=True)

          order = readFullInheritance(taginfo['id'], event=event)

          # follow inheritance for arches and extra

          for link in order:

              if link['noconfig']:

                  continue

-             ancestor = get_tag(link['parent_id'], strict=True, event=event)

+             ancestor = get_tag(link['parent_id'], strict=True, event=event, blocked=True)

              if taginfo['arches'] is None and ancestor['arches'] is not None:

                  taginfo['arches'] = ancestor['arches']

              for key in ancestor['extra']:

                  if key not in taginfo['extra']:

                      taginfo['extra'][key] = ancestor['extra'][key]

+         # cleanup extras by blocked

+         for k, v in list(taginfo['extra'].items()):

+             if v[0]:

+                 del taginfo['extra'][k]

+             else:

+                 taginfo['extra'][k] = v[1]

          return taginfo

  

      def getRepo(self, tag, state=None, event=None, dist=False):

@@ -46,6 +46,7 @@ 

                  'maven_support': maven_support,

                  'maven_include_all': maven_include_all,

                  'extra': extra,

+                 'block_extra': [],

                  'remove_extra': remove_extra}

          options = mock.MagicMock()

  
@@ -78,6 +79,8 @@ 

          opts = {'perm': None,

                  'locked': not locked,

                  'maven_support': not maven_support,

+                 'block_extra': [],

+                 'remove_extra': [],

                  'maven_include_all': not maven_include_all}

          # Run it and check immediate output

          # args: tag --no-perm --unlock --no-maven-support --no-include-all
@@ -130,6 +133,8 @@ 

                          Set tag extra option

    -r key, --remove-extra=key

                          Remove tag extra option

+   -b key, --block-extra=key

+                         Block inherited tag extra option

  """ % progname

          expected_stderr = ''

          self.assertMultiLineEqual(actual_stdout, expected_stdout)

WIP: I've converted column to jsonb which allow us to differentiate between SQL NULL and jsonb null. Anyway, current state lacks an ability for user to find which tag is responsible for blocking (blocked values simple got lost). I'll wait for #2493 resolution as it is interlinked a bit.

which allow us to differentiate between SQL NULL and jsonb null.

There are several reasons that jsonb would be useful (mostly speed and more powerful querying), but I don't think this is one of them. We could allow a null value (or perhaps just an empty string value) in the existing text field.

rebased onto 9e1edb884415b70fb3c89c4905e74d468e845341

9 months ago

Yep, I was overengineering, that we would like to differentiate between SQL NULL and None value in the field. So, mock.new_chroot: None would be different to mock.new_chroot: NULL where first one says extra option exists and is set to None while the second says it is blocked. But it probably has not much real use and we can interpret missing value as None always.

rebased onto 9d561ae19e1262e5d2480c8d424ff4a14e02a04b

9 months ago

rebased onto 8d3fdc7ab72971de7f82aa7179ffe6ac17daa282

9 months ago

rebased onto f5222ac

9 months ago

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

9 months ago

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

9 months ago

Commit 5e8ed0c fixes this pull-request

Pull-Request has been merged by tkopecek

9 months ago