This patch allows sending fedmsg with whatever is sent from libtaskotron check as a result.
Details
Details
When deployed on dev/stg
Diff Detail
Diff Detail
- Repository
- rRSDB resultsdb
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| resultsdb/controllers/api_v1.py | ||
|---|---|---|
| 627 | It feels like passing just the result object would look/be better - you do it just one line lower in the get_fedmsg call anyway... Also, not all of the results stored in ResultsDB need to have the result_data[item] specified, so you might at least want to change it to args['results_data'].get('item', None) | |
| 643 | Might be worth adding an explicit sort, just to make sure that the first is really the "newest". I'm not really sure what's the default way of sorting data for queries, but my guess would be primary_key - ascending, which would make the first actually the oldest, rather than newest. | |
| 649 | Maybe rename to create_fedmsg or something like that? I don't really care that much, though. | |
| 656 | I most probably missed the fedmsg structure discussion (so feel free to ignore the comment), but wouldn't it be better to fill check just with check['name'], and put the rest of the data - which at the moment, since you take it from result_data, is "everything the check developer decided to store", and accidentally also item, type and the likes - to a separate key? | |
| conf/settings.py.example | ||
|---|---|---|
| 13–14 | Please document the settings (I have no idea what "modname" is). Furthermore, you haven't added them to resultsdb/config.py. Doesn't that mean that the code will crash for users without conf/settings.py file (or with the values commented out)? | |
| resultsdb/controllers/api_v1.py | ||
| 627 | Btw, None is the default fallback value for dict.get(). | |
| 634 | It would be nice to have a docstring describing what exactly is considered duplicate. It will give us better assurance in the future when we decide to modify the code, that it's still doing what was intended. | |
| 657 | Why is this empty? Does it have to be there when it is empty? | |
| 660 | Would it be a good idea to encode the timezone info as well, and not just suppose they'll assume it's UTC? | |
Comment Actions
| resultsdb.spec | ||
|---|---|---|
| 25 | it would be wise to start versioning our deps in resultsdb like we did in libtaskotron | |
| resultsdb/controllers/api_v1.py | ||
| 656 | I must have missed this during the fedmsg structure discussion but this should probably be task instead of check since we'll be emitting results for more than just checks | |
| resultsdb/controllers/api_v1.py | ||
|---|---|---|
| 656 |
In my mind, the check key (now task) is input for the task: its name and under what conditions the task was run. The result key then is output of the task. Does that make sense? OTOH, you do have a point that result_data can be filled also with data other than input, like additional info about the result of the task that the task developer wants to store. In that case, would it make sense to store input parts of result_data into task and the rest of it into result? The thing I don't like about it, though, is that we'd need to do some hardcoding (item, type, ...). Thoughts? | |
Comment Actions
Are we stuck on waiting for more acks on this review or is there something else I'm missing?
| resultsdb/controllers/api_v1.py | ||
|---|---|---|
| 670 | Can you please publish a real-world example for the whole fedmsg? If I read the code correctly, task should contain item, type, log_url (which would be duplicated with results) and any extra keyvals the task defined. Out of these, I think only item and type should be there. Unless we have a real use case right now, I'd avoid including all extra keyvals directly in the fedmsg. We want to keep the fedmsg small and we have no idea how large the extra keyvals might be (we have even placed no restriction on it in libtaskotron, not sure about resultsdb). Therefore I'd really hand-pick ("hardcode") a few items that we want to publish here, as mentioned in one of the previous comments. | |
| 675 | Since the switch to ResultYAML, we have note instead of summary, so this should probably reflect that. I don't know whether the field name in database changed as well, or if it is still summary, @jskladan will know. But if we're trying to keep the fedmsgs small in size, I don't think we need to publish at all. It's an extra piece of data, but it doesn't have to be a part of the notification, if you need that, you'll probably need to query for the full result anyway. I'd leave it out. | |
| resultsdb/controllers/api_v1.py | ||
|---|---|---|
| 670 | {'modname': 'taskotron',
'msg': {'result': {'id': 481,
'job_url': u'http://localhost:5003/jobs/20151005_094935_437061',
'log_url': u'http://localhost/artifacts/all/20151005_094935_437061/task_output/openstack-cinder-7.0.0-0.1.0rc1.fc24.log',
'outcome': 'FAILED',
'submit_time': '2015-10-05 09:49:48 UTC',
'summary': u'rpmlint FAILED for openstack-cinder-7.0.0-0.1.0rc1.fc24'},
'task': {u'item': u'openstack-cinder-7.0.0-0.1.0rc1.fc24',
'name': u'rpmlint',
u'type': u'koji_build'}},
'topic': 'result.new'}I am not sure why you think that log_url is in result_data. Anyway I agree with having only item and type from result_data in task. I'll update the patch shortly. | |
| 675 | I think that name was changed only in CheckDetail in libtaskotron, wasn't it? Either way, I think we don't need to include that in fedmsgs, yeah. | |
Comment Actions
I think the current fedmsg structure looks both useful and concise. Thanks.
| resultsdb/controllers/api_v1.py | ||
|---|---|---|
| 670 | Because there is no documentation, so I looked up that result_data is created from extra_data, and extra_data have this comment: # fill extra_data with the query parameters 'other' than those defined in RP['get_results'] And RP['get_results'] doesn't define log_url, but we send it from resultsdb_directive. Phew. But it seems I made a mistake somewhere or the comment is not precise. That's why I asked for a real-world example :) | |
Please document the settings (I have no idea what "modname" is). Furthermore, you haven't added them to resultsdb/config.py. Doesn't that mean that the code will crash for users without conf/settings.py file (or with the values commented out)?