Differential D414

resultsdb_directive: always return TAP
ClosedPublic
Actions

Authored by kparal on Jul 3 2015, 3:39 PM.

Details

Reviewers
lbrabec
tflink
mkrizek
jskladan
Commits
rLTRNbc4c65a1eec7: resultsdb_directive: always return TAP
Summary

In 39a62f7a1 we started relying on exporting the output from
resultsdb_directive and forwarding it to other directives
(bodhi_comments_directive). That breaks if reporting to ResultsDB is
disabled. This patch fixes that.

Test Plan

test suite works, and my manual task execution as well (with bodhi comments enabled, but resultsdb reporting disabled)

Diff Detail

Repository
rLTRN libtaskotron
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kparal retitled this revision from to resultsdb_directive: always return TAP.Jul 3 2015, 3:39 PM
kparal updated this object.
kparal edited the test plan for this revision. (Show Details)
kparal added reviewers: jskladan, mkrizek, tflink, lbrabec.
tflink added a comment.Jul 7 2015, 3:53 AM

Looks like it'll work but wouldn't it be better if we replicated the return statement that's used if resultsdb reporting is enabled instead of doing something different? The check detail objects aren't altered during the reporting process and they're already constructed bythe time that we get to the return statements

libtaskotron/directives/resultsdb_directive.py
211

Why return the input data here instead of replicating the return check.export_TAP(check_details) call that's made if resultsdb reporting is enabled?

kparal added inline comments.Jul 7 2015, 10:22 AM
libtaskotron/directives/resultsdb_directive.py
211

My original (very quick) thought was that when reporting is disabled, I'd return the raw unaltered input data, behaving as a "no-op" operation. If I import and re-export TAP, I'll return different data (reordered, reformatted, unknown fields ignored). I'd be basically sanitizing the data to some extent.

But thinking about it more, it's probably somewhat weird for this directive to return sometimes raw input content, and sometimes sanitized content, depending on results having been reported or not. Consistent output behavior (sans the result id field) seems like a better thing to do. Good point. I'll adjust accordingly.

kparal updated this revision to Diff 1106.Jul 7 2015, 10:56 AM

change according to @tflink's comments

tflink accepted this revision.Jul 8 2015, 4:14 AM
This revision is now accepted and ready to land.Jul 8 2015, 4:14 AM
Closed by commit rLTRNbc4c65a1eec7: resultsdb_directive: always return TAP (authored by kparal). · Explain WhyJul 8 2015, 7:59 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

Diff 1107

View Options

libtaskotron/directives/resultsdb_directive.py

Show All 20 Lines
21parameters: 21​parameters:
22 results: 22​ results:
23 required: true 23​ required: true
24 description: The output of the check in TAP13 format 24​ description: The output of the check in TAP13 format
25 type: str 25​ type: str
26returns: | 26​returns: |
27 TAP13 formatted output as taken from input, enhanced with result-id's for each 27​ TAP13 formatted output as taken from input, enhanced with result-id's for each
28 successfully stored result. The result-id information then can be used by 28​ successfully stored result. The result-id information then can be used by
29 other directives - e.g. ``bodhi_comment_directive``. 29​ other directives - e.g. ``bodhi_comment_directive``. If reporting to ResultsDB
30​ is disabled, the returned TAP does not contain this extra field.
30raises: | 31​raises: |
31 * :class:`.TaskotronDirectiveError`: if ``results`` is not in a valid TAP13 32​ * :class:`.TaskotronDirectiveError`: if ``results`` is not in a valid TAP13
32 format, or when ``item`` or ``type`` are missing in the TAP's result data 33​ format, or when ``item`` or ``type`` are missing in the TAP's result data
33 * :class:`resultsdb_api.ResultsDBapiException`: for any errors coming from 34​ * :class:`resultsdb_api.ResultsDBapiException`: for any errors coming from
34 the ResultsDB server 35​ the ResultsDB server
35version_added: 0.4 36​version_added: 0.4
36""" 37​"""
37 38​
▲ Show 20 Lines • Show All 164 Lines • ▼ Show 20 Line(s)183 def process(self, input_data, env_data):
202 203​
203 conf = config.get_config() 204​ conf = config.get_config()
204 if not (conf.reporting_enabled and conf.report_to_resultsdb): 205​ if not (conf.reporting_enabled and conf.report_to_resultsdb):
205 log.info("Reporting to ResultsDB is disabled. Once enabled, the " 206​ log.info("Reporting to ResultsDB is disabled. Once enabled, the "
206 "following would get reported:\n%s" % 207​ "following would get reported:\n%s" %
207 report_summary(input_data['results'], env_data['checkname'])) 208​ report_summary(input_data['results'], env_data['checkname']))
208 log.info('Hint: Enabling debug output allows you to see unstripped ' 209​ log.info('Hint: Enabling debug output allows you to see unstripped '
209 'values during variable export.') 210​ 'values during variable export.')
210 return 211​ return check.export_TAP(check_details)
tflinkUnsubmitted
Not Done

Why return the input data here instead of replicating the return check.export_TAP(check_details) call that's made if resultsdb reporting is enabled?

kparalAuthorUnsubmitted
Not Done

My original (very quick) thought was that when reporting is disabled, I'd return the raw unaltered input data, behaving as a "no-op" operation. If I import and re-export TAP, I'll return different data (reordered, reformatted, unknown fields ignored). I'd be basically sanitizing the data to some extent.

But thinking about it more, it's probably somewhat weird for this directive to return sometimes raw input content, and sometimes sanitized content, depending on results having been reported or not. Consistent output behavior (sans the result id field) seems like a better thing to do. Good point. I'll adjust accordingly.

211 212​
212 # for now, we're creating the resultsdb job at reporting time 213​ # for now, we're creating the resultsdb job at reporting time
213 # the job needs to be 'RUNNING' in order to append any results 214​ # the job needs to be 'RUNNING' in order to append any results
214 job_url, log_url = buildbot_utils.get_urls(env_data['jobid'], 215​ job_url, log_url = buildbot_utils.get_urls(env_data['jobid'],
215 self.masterurl, 216​ self.masterurl,
216 self.task_stepname) 217​ self.task_stepname)
217 job_data = self.create_resultsdb_job(env_data['checkname'], 218​ job_data = self.create_resultsdb_job(env_data['checkname'],
218 uuid=env_data['uuid'], 219​ uuid=env_data['uuid'],
▲ Show 20 Lines • Show All 69 Lines • Show Last 20 Lines
View Options

testing/test_resultsdb_directive.py

Show All 29 Lines29 self.cd = check.CheckDetail(
30 item = 'foo_bar', 30​ item = 'foo_bar',
31 report_type = check.ReportType.KOJI_BUILD, 31​ report_type = check.ReportType.KOJI_BUILD,
32 outcome = 'NEEDS_INSPECTION', 32​ outcome = 'NEEDS_INSPECTION',
33 summary = 'foo_bar summary', 33​ summary = 'foo_bar summary',
34 output = ["foo\nbar"], 34​ output = ["foo\nbar"],
35 keyvals = {"foo": "moo1", "bar": "moo2"}, 35​ keyvals = {"foo": "moo1", "bar": "moo2"},
36 ) 36​ )
37 37​
38​ self.tap = check.export_TAP(self.cd)
38 39​
39 tap = check.export_TAP(self.cd) 40​ self.ref_input = {'results': self.tap}
40 ​
41 self.ref_input = {'results': tap} ​
42 self.ref_envdata = { 41​ self.ref_envdata = {
43 'resultsdb_job_id': 1, 42​ 'resultsdb_job_id': 1,
44 'checkname': 'test_resultsdb_report', 43​ 'checkname': 'test_resultsdb_report',
45 'jobid': 'all/123', 44​ 'jobid': 'all/123',
46 'uuid': 'c25237a4-b6b3-11e4-b98a-3c970e018701', 45​ 'uuid': 'c25237a4-b6b3-11e4-b98a-3c970e018701',
47 'artifactsdir': '/some/directory/', 46​ 'artifactsdir': '/some/directory/',
48 } 47​ }
49 48​
Show All 24 Lines
74 self.conf.report_to_resultsdb = True 73​ self.conf.report_to_resultsdb = True
75 74​
76 75​
77 def test_config_reporting_disabled(self): 76​ def test_config_reporting_disabled(self):
78 """Checks config options can disable reporting.""" 77​ """Checks config options can disable reporting."""
79 conf = config.get_config() 78​ conf = config.get_config()
80 79​
81 for r_e, r_t_r in ((False, False), (False, True), (True, False)): 80​ for r_e, r_t_r in ((False, False), (False, True), (True, False)):
82 conf.reporting_enabled = r_e 81​ conf.reporting_enabled = r_e
83 conf.report_to_resultsdb = r_t_r 82​ conf.report_to_resultsdb = r_t_r
83​
84​ tap = self.test_rdb.process(self.ref_input, self.ref_envdata)
85​ cds = check.import_TAP(tap)
86​ # return value should be the same TAP
87​ assert len(cds) == 1
88​ assert cds[0].__dict__ == self.cd.__dict__
84 89​
85 assert self.test_rdb.process(self.ref_input, self.ref_envdata) is None 90​ # no call should have been made
91​ assert len(self.stub_rdb.calls()) == 0
86 92​
87 config._config = None 93​ config._config = None
88 94​
89 def test_missing_checkname(self): 95​ def test_missing_checkname(self):
90 """Checks if missing checkname raises exception""" 96​ """Checks if missing checkname raises exception"""
91 with pytest.raises(TaskotronDirectiveError): 97​ with pytest.raises(TaskotronDirectiveError):
92 envdata = deepcopy(self.ref_envdata) 98​ envdata = deepcopy(self.ref_envdata)
93 del(envdata['checkname']) 99​ del(envdata['checkname'])
▲ Show 20 Lines • Show All 206 Lines • Show Last 20 Lines