PR#2861: Add the ability to specify custom metadata on an RPM build - koji

koji

#2861 Add the ability to specify custom metadata on an RPM build

Closed 3 years ago by tkopecek. Opened 4 years ago by mprahl.

mprahl/koji custom-user-metadata into master

Add the ability to specify custom metadata on an RPM build

mprahl • 3 years ago

4affdfb

builder/kojid

file modified

+17

		`@@ -27,6 +27,7 @@`
		`import glob`
		`import grp`
		`import io`
		`+ import json`
		`import logging`
		`import logging.handlers`
		`import os`
		`@@ -962,6 +963,18 @@`
		`repo_info = None`
		`# we'll wait for a repo later (self.getRepo)`
		`self.event_id = None`
		`+`
		`+ if opts.get('custom_user_metadata'):`
		`+ if not isinstance(opts['custom_user_metadata'], dict):`
		`+ raise koji.BuildError('custom_user_metadata must be serializable to a JSON object')`
		`+`
		`+ try:`
		`+ json.dumps(opts['custom_user_metadata'])`
		`+ except TypeError:`
		`+ error_msg = 'custom_user_metadata is not JSON serializable'`
		`+ self.logger.exception(error_msg)`
		`+ raise koji.BuildError(error_msg)`
		`+`
		`task_info = self.session.getTaskInfo(self.id)`
		`target_info = None`
		`if target:`
		`@@ -1019,6 +1032,10 @@`
		`data['source'] = self.source['source']`
		`data['extra'] = {'source': {'original_url': self.source['url']}}`

		`+ if opts.get('custom_user_metadata'):`
		`+ data.setdefault('extra', {})`
		`+ data['extra']['custom_user_metadata'] = opts['custom_user_metadata']`
		`+`
		`extra_arches = None`
		`self.logger.info("Reading package config for %(name)s" % data)`
		`pkg_cfg = self.session.getPackageConfig(dest_tag, data['name'], event=self.event_id)`

cli/koji_cli/commands.py

file modified

+15

		`@@ -494,6 +494,9 @@`
		`help=_("Do not display progress of the upload"))`
		`parser.add_option("--background", action="store_true",`
		`help=_("Run the build at a lower priority"))`
		`+ parser.add_option("--custom-user-metadata", type="str",`
		`+ help=_("Provide a JSON string of custom metadata to be deserialized and "`
		`+ "stored under the build's extra.custom_user_metadata field"))`
		`(build_opts, args) = parser.parse_args(args)`
		`if len(args) != 2:`
		`parser.error(_("Exactly two arguments (a build target and a SCM URL or srpm file) are "`
		`@@ -502,6 +505,17 @@`
		`parser.error(_("--no-/rebuild-srpm is only allowed for --scratch builds"))`
		`if build_opts.arch_override and not build_opts.scratch:`
		`parser.error(_("--arch_override is only allowed for --scratch builds"))`
		`+ custom_user_metadata = {}`
		`+ if build_opts.custom_user_metadata:`
		`+ try:`
		`+ custom_user_metadata = json.loads(build_opts.custom_user_metadata)`
		`+ # Use ValueError instead of json.JSONDecodeError for Python 2 and 3 compatibility`
		`+ except ValueError:`
		`+ parser.error(_("--custom-user-metadata is not valid JSON"))`
		`+`
		`+ if not isinstance(custom_user_metadata, dict):`
		`+ parser.error(_("--custom-user-metadata must be a JSON object"))`
		`+`
		`activate_session(session, options)`
		`target = args[0]`
		`if target.lower() == "none" and build_opts.repo_id:`
		`@@ -525,6 +539,7 @@`
		`val = getattr(build_opts, key)`
		`if val is not None:`
		`opts[key] = val`
		`+ opts["custom_user_metadata"] = custom_user_metadata`
		`priority = None`
		`if build_opts.background:`
		`# relative to koji.PRIO_DEFAULT`

tests/test_cli/test_build.py

file modified

+150 -10

		`@@ -52,7 +52,7 @@`
		`source = 'srpm'`
		`task_id = 1`
		`args = [target, source]`
		`- opts = {'wait_builds': []}`
		`+ opts = {'custom_user_metadata': {}, 'wait_builds': []}`
		`priority = None`

		`self.session.getBuildTarget.return_value = target_info`
		`@@ -105,7 +105,7 @@`
		`source = 'http://scm'`
		`task_id = 1`
		`args = [target, source]`
		`- opts = {'wait_builds': []}`
		`+ opts = {'custom_user_metadata': {}, 'wait_builds': []}`
		`priority = None`

		`self.session.getBuildTarget.return_value = target_info`
		`@@ -226,6 +226,10 @@`
		`--repo-id=REPO_ID Use a specific repo`
		`--noprogress Do not display progress of the upload`
		`--background Run the build at a lower priority`
		`+ --custom-user-metadata=CUSTOM_USER_METADATA`
		`+ Provide a JSON string of custom metadata to be`
		`+ deserialized and stored under the build's`
		`+ extra.custom_user_metadata field`
		`""" % (progname, progname)`
		`expected_stderr = ''`
		`self.assertMultiLineEqual(actual_stdout, expected_stdout)`
		`@@ -243,6 +247,135 @@`
		`watch_tasks_mock.assert_not_called()`

		`@mock.patch('sys.stdout', new_callable=six.StringIO)`
		`+ @mock.patch('koji_cli.commands.activate_session')`
		`+ @mock.patch('koji_cli.commands.unique_path', return_value='random_path')`
		`+ @mock.patch('koji_cli.commands._running_in_bg', return_value=False)`
		`+ @mock.patch('koji_cli.commands.watch_tasks', return_value=0)`
		`+ def test_handle_build_custom_user_metadata(`
		`+ self,`
		`+ watch_tasks_mock,`
		`+ running_in_bg_mock,`
		`+ unique_path_mock,`
		`+ activate_session_mock,`
		`+ stdout):`
		`+ target = 'target'`
		`+ dest_tag = 'dest_tag'`
		`+ target_info = {'dest_tag': dest_tag}`
		`+ dest_tag_info = {'locked': False}`
		`+ source = 'http://scm'`
		`+ task_id = 1`
		`+ args = ['--custom-user-metadata={"automation-triggered-by": "yoda"}', target, source]`
		`+ opts = {'custom_user_metadata': {'automation-triggered-by': 'yoda'}, 'wait_builds': []}`
		`+ priority = None`
		`+`
		`+ self.session.getBuildTarget.return_value = target_info`
		`+ self.session.getTag.return_value = dest_tag_info`
		`+ self.session.build.return_value = task_id`
		`+ # Run it and check immediate output`
		`+ # args: target, http://scm`
		`+ # expected: success`
		`+ rv = handle_build(self.options, self.session, args)`
		`+ actual = stdout.getvalue()`
		`+ expected = """Created task: 1`
		`+ Task info: weburl/taskinfo?taskID=1`
		`+ """`
		`+ self.assertMultiLineEqual(actual, expected)`
		`+ # Finally, assert that things were called as we expected.`
		`+ activate_session_mock.assert_called_once_with(self.session, self.options)`
		`+ self.session.getBuildTarget.assert_called_once_with(target)`
		`+ self.session.getTag.assert_called_once_with(dest_tag)`
		`+ unique_path_mock.assert_not_called()`
		`+ running_in_bg_mock.assert_called_once()`
		`+ self.session.uploadWrapper.assert_not_called()`
		`+ self.session.build.assert_called_once_with(`
		`+ source, target, opts, priority=priority)`
		`+ self.session.logout.assert_called()`
		`+ watch_tasks_mock.assert_called_once_with(`
		`+ self.session, [task_id], quiet=self.options.quiet,`
		`+ poll_interval=self.options.poll_interval, topurl=self.options.topurl)`
		`+ self.assertEqual(rv, 0)`
		`+`
		`+ @mock.patch('sys.stdout', new_callable=six.StringIO)`
		`+ @mock.patch('sys.stderr', new_callable=six.StringIO)`
		`+ @mock.patch('koji_cli.commands.activate_session')`
		`+ @mock.patch('koji_cli.commands.unique_path', return_value='random_path')`
		`+ @mock.patch('koji_cli.commands._running_in_bg', return_value=False)`
		`+ @mock.patch('koji_cli.commands.watch_tasks', return_value=0)`
		`+ def test_handle_build_custom_user_metadata_invalid_json(`
		`+ self,`
		`+ watch_tasks_mock,`
		`+ running_in_bg_mock,`
		`+ unique_path_mock,`
		`+ activate_session_mock,`
		`+ stderr,`
		`+ stdout):`
		`+ target = 'target'`
		`+ source = 'http://scm'`
		`+ args = [target, source, '--custom-user-metadata={Do or do not. There is no try.}']`
		`+`
		`+ # Run it and check immediate output`
		`+ with self.assertRaises(SystemExit) as ex:`
		`+ handle_build(self.options, self.session, args)`
		`+ self.assertExitCode(ex, 2)`
		`+ actual_stdout = stdout.getvalue()`
		`+ actual_stderr = stderr.getvalue()`
		`+ expected_stdout = ''`
		`+ expected_stderr = self.format_error_message("--custom-user-metadata is not valid JSON")`
		`+ self.assertMultiLineEqual(actual_stdout, expected_stdout)`
		`+ self.assertMultiLineEqual(actual_stderr, expected_stderr)`
		`+`
		`+ # Finally, assert that things were called as we expected.`
		`+ activate_session_mock.assert_not_called()`
		`+ self.session.getBuildTarget.assert_not_called()`
		`+ self.session.getTag.assert_not_called()`
		`+ unique_path_mock.assert_not_called()`
		`+ running_in_bg_mock.assert_not_called()`
		`+ self.session.uploadWrapper.assert_not_called()`
		`+ self.session.build.assert_not_called()`
		`+ self.session.logout.assert_not_called()`
		`+ watch_tasks_mock.assert_not_called()`
		`+`
		`+ @mock.patch('sys.stdout', new_callable=six.StringIO)`
		`+ @mock.patch('sys.stderr', new_callable=six.StringIO)`
		`+ @mock.patch('koji_cli.commands.activate_session')`
		`+ @mock.patch('koji_cli.commands.unique_path', return_value='random_path')`
		`+ @mock.patch('koji_cli.commands._running_in_bg', return_value=False)`
		`+ @mock.patch('koji_cli.commands.watch_tasks', return_value=0)`
		`+ def test_handle_build_custom_user_metadata_not_json_object(`
		`+ self,`
		`+ watch_tasks_mock,`
		`+ running_in_bg_mock,`
		`+ unique_path_mock,`
		`+ activate_session_mock,`
		`+ stderr,`
		`+ stdout):`
		`+ target = 'target'`
		`+ source = 'http://scm'`
		`+ args = [target, source, '--custom-user-metadata="Do or do not. There is no try."']`
		`+`
		`+ # Run it and check immediate output`
		`+ with self.assertRaises(SystemExit) as ex:`
		`+ handle_build(self.options, self.session, args)`
		`+ self.assertExitCode(ex, 2)`
		`+ actual_stdout = stdout.getvalue()`
		`+ actual_stderr = stderr.getvalue()`
		`+ expected_stdout = ''`
		`+ expected_stderr = self.format_error_message("--custom-user-metadata must be a JSON object")`
		`+ self.assertMultiLineEqual(actual_stdout, expected_stdout)`
		`+ self.assertMultiLineEqual(actual_stderr, expected_stderr)`
		`+`
		`+ # Finally, assert that things were called as we expected.`
		`+ activate_session_mock.assert_not_called()`
		`+ self.session.getBuildTarget.assert_not_called()`
		`+ self.session.getTag.assert_not_called()`
		`+ unique_path_mock.assert_not_called()`
		`+ running_in_bg_mock.assert_not_called()`
		`+ self.session.uploadWrapper.assert_not_called()`
		`+ self.session.build.assert_not_called()`
		`+ self.session.logout.assert_not_called()`
		`+ watch_tasks_mock.assert_not_called()`
		`+`
		`+ @mock.patch('sys.stdout', new_callable=six.StringIO)`
		`@mock.patch('sys.stderr', new_callable=six.StringIO)`
		`@mock.patch('koji_cli.commands.activate_session')`
		`@mock.patch('koji_cli.commands.unique_path', return_value='random_path')`
		`@@ -301,7 +434,9 @@`
		`task_id = 1`
		`repo_id = 2`
		`args = ['--repo-id=' + str(repo_id), target, source]`
		`- opts = {'repo_id': repo_id, 'skip_tag': True, 'wait_builds': []}`
		`+ opts = {`
		`+ 'repo_id': repo_id, 'skip_tag': True, 'wait_builds': [], 'custom_user_metadata': {}`
		`+ }`
		`priority = None`

		`self.session.build.return_value = task_id`
		`@@ -483,7 +618,12 @@`
		`'--scratch',`
		`target,`
		`source]`
		`- opts = {'arch_override': arch_override, 'scratch': True, 'wait_builds': []}`
		`+ opts = {`
		`+ 'arch_override': arch_override,`
		`+ 'custom_user_metadata': {},`
		`+ 'scratch': True,`
		`+ 'wait_builds': [],`
		`+ }`
		`priority = None`

		`self.session.getBuildTarget.return_value = target_info`
		`@@ -534,7 +674,7 @@`
		`task_id = 1`
		`args = ['--background', target, source]`
		`priority = 5`
		`- opts = {'wait_builds': []}`
		`+ opts = {'custom_user_metadata': {}, 'wait_builds': []}`

		`self.session.getBuildTarget.return_value = target_info`
		`self.session.getTag.return_value = dest_tag_info`
		`@@ -582,7 +722,7 @@`
		`source = 'srpm'`
		`task_id = 1`
		`args = [target, source]`
		`- opts = {'wait_builds': []}`
		`+ opts = {'custom_user_metadata': {}, 'wait_builds': []}`
		`priority = None`

		`self.session.getBuildTarget.return_value = target_info`
		`@@ -633,7 +773,7 @@`
		`source = 'srpm'`
		`task_id = 1`
		`args = ['--noprogress', target, source]`
		`- opts = {'wait_builds': []}`
		`+ opts = {'custom_user_metadata': {}, 'wait_builds': []}`
		`priority = None`

		`self.session.getBuildTarget.return_value = target_info`
		`@@ -687,7 +827,7 @@`
		`task_id = 1`
		`quiet = True`
		`args = ['--quiet', target, source]`
		`- opts = {'wait_builds': []}`
		`+ opts = {'custom_user_metadata': {}, 'wait_builds': []}`
		`priority = None`

		`self.session.getBuildTarget.return_value = target_info`
		`@@ -737,7 +877,7 @@`
		`task_id = 1`
		`quiet = None`
		`args = ['--wait', target, source]`
		`- opts = {'wait_builds': []}`
		`+ opts = {'custom_user_metadata': {}, 'wait_builds': []}`
		`priority = None`

		`self.session.getBuildTarget.return_value = target_info`
		`@@ -790,7 +930,7 @@`
		`source = 'srpm'`
		`task_id = 1`
		`args = ['--nowait', target, source]`
		`- opts = {'wait_builds': []}`
		`+ opts = {'custom_user_metadata': {}, 'wait_builds': []}`
		`priority = None`

		`self.session.getBuildTarget.return_value = target_info`

mprahl commented 4 years ago

This adds the --custom-user-metadata option to the
CLI build command. This is then stored under the
"extra.custom_user_metadata" field on the resulting build.

mprahl commented 4 years ago

@mikem, @tkopecek, @breilly could you please review?

I couldn't find any existing unit tests for BuildTask.handler. If there are in fact some, could you please point me in the right direction to add to them?

tkopecek commented on line 37 of builder/kojid 3 years ago

It is in 'if source' branch. So, it wouldn't work correctly in some cases. Move it out of the 'if'. + I wouldn't add an empty dict if option was not provided. Most of the builds will not have it anyway (we're probably will not be patching the old records).

tkopecek commented 3 years ago

code is not py2.7 compatible (see jenkins logs) - at least json.JSONDecodeError doesn't exist there.

rebased onto 4affdfb

3 years ago

mprahl commented 3 years ago

@tkopecek thanks for the review! I addressed your comments. Could you please take another look?

tkopecek commented 3 years ago

:thumbsup: @mikem ?

tkopecek commented 3 years ago

Added #2866 for tracking.

mikem commented 3 years ago

Since this is intended for automation, do we need to add the cli option?

mikem commented 3 years ago

Since this is intended for automation, do we need to add the cli option?

Nm, I don't see how it could hurt

mikem commented 3 years ago

:thumbsup:

mprahl commented 3 years ago

Thanks for the reviews @tkopecek and @mikem! Is this ready to be merged?

tkopecek commented 3 years ago

Basically yes, now we're in code freeze, so after release (probably this Thursday) it will get testing-ready flag, so our QE can look at it. After getting QA ack, it will be merged.

mprahl commented 3 years ago

Thanks for the clarification!