Show bug status in openQA test result matrix #1292
Conversation
asdil12
force-pushed
the
bugs
branch
3 times, most recently
from
98c88da
to
69936a2
Compare
lib/OpenQA/Schema/Result/Bugs.pm
Outdated
| if ($self->assignee eq 'None') { | ||
| return 0; | ||
| } | ||
| elsif ($self->assignee =~ m/\@forge\.provo\.novell\.com/) { |
There was a problem hiding this comment.
no. The progress issue is about displaying the status of the issue/bug within the dashboard. For that we don't need to duplicate bugzilla - or worse: hardcode specific logic about novell assignees.
OpenQA's database can hold the informations necessary to pick the icon - but which icon to display belongs in SUSE specific scripts using some API. I don't want to see bugzilla fetching scripts within openQA. And I thought I made myself clear about that in the past.
There was a problem hiding this comment.
so do you want to a field "is_assigned" in the database instead of the sub?
There was a problem hiding this comment.
Wouldn't that be redundancy?
There was a problem hiding this comment.
I owe @coolo many beers for his stalwart stance against Novell-isms ;) keep fighting the good fight!
|
On Friday, 7 April 2017 11:37:54 CEST Stephan Kulow wrote:
[…]
OpenQA's database can hold the informations necessary to pick the icon - but
which icon to display belongs in SUSE specific scripts using some API. I
don't want to see bugzilla fetching scripts within openQA. And I thought I
made myself clear about that in the past.
|
lib/OpenQA/Schema/Result/Bugs.pm
Outdated
| } | ||
| else { | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
What's with all this verbosity? Why not just
sub is_open {
return shift->status !~ m/(RESOLVED|REJECTED|VERIFIED|CLOSED)/i;
}
lib/OpenQA/Schema/Result/Bugs.pm
Outdated
| } | ||
| else { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Also here I would use early return on error instead of lines L96-L101
return 0 unless $res->is_success;
return $res->json;
lib/OpenQA/Schema/Result/Bugs.pm
Outdated
| if ($self->assignee eq 'None') { | ||
| return 0; | ||
| } | ||
| elsif ($self->assignee =~ m/\@forge\.provo\.novell\.com/) { |
There was a problem hiding this comment.
so do you want to a field "is_assigned" in the database instead of the sub?
|
as long as https://progress.opensuse.org/issues/12092 does not have a good description what we want to achieve with that I do not want to approve the PR. |
|
@okurz happy now? |
|
cool, yes. But this PR does not fully cover this, yet, right? |
|
@okurz |
|
@okurz remember "commit early and often"? I remember some guy making a lot about that..very often ;) |
|
if this does not mean 'merge pre-maturely' I'm fine. Because I really don't see this bugzilla fetching in openQA |
|
Understood, I consider this PR successful because @asdil12 should now have a better unstanding which bits need to go in openQA and which will be populated by an external script - but I still think we need this PR to provide the DB changes so the script has somewhere to put the necessary data regarding bugs so we can display the appropriate icons. |
|
yes yes. "commit early, commit often" applies. I was just asking :-) |
|
Let me give you a TODO list from point of view:
|
|
https://github.com/python-bugzilla/python-bugzilla is pretty good, and we (RH and Fedora) use it for tons of stuff so it's very actively maintained. |
|
@AdamWill Thanks, but I already have a Python (see openqa_review) and a Perl (see this PR) implementation of bugzilla/redmine bug status fetcher. |
|
@coolo Setting bug status via an external tool using an api in openQA would require us to fetch a list of all bugs from openQA. Also I don't think, that writing an API with authentication and running an external tool is worth the effort for code that takes <100 loc. |
|
Your concern was noted. Now write the API |
|
"Also the approach with an tool would prevent live update when a new bug is added to openQA. This isn't necessarily true, is it? IIRC, you guys have a message queue system, right? So you could just have openQA emit an event to that queue whenever a bug is 'added' to openQA, and your external system could listen for those events and immediately do its job. You don't have to only run it once a day or something. In fact, openQA is probably already set up to emit messages on your bus thing whenever a comment is created or updated; your external system could just listen out for those messages and activate any time it sees a comment text with a bugref in it. Though I suppose that requires duplicating the bugref code. |
No, it isn't.
Yes, we do. AMQP. It's enabled on the instances we have.
correct. And @asdil12 was the main guy to set this up so he should know and not come up with that lazy excuse ;-) @asdil12 API + triggering based on AMQP events sounds right. I don't consider it huge effort. Take a look into https://github.com/okurz/scripts/blob/master/openqa_label_all_tests_with_one_failing_modules#L77 as a "lazy" approach to use the openQA client from within python. And I guess https://github.com/openSUSE/osc-plugin-factory/blob/master/openqa-comments.py and https://github.com/openSUSE/osc-plugin-factory/blob/master/totest-manager.py are also good references how to use the openQA API in that regard |
|
@okurz you remember I wrote an Python client library, right? https://github.com/os-autoinst/openQA-python-client ... it reads the same config files as the perl client, so you just have to do something like: and then you can make requests like: where 'GET' is the method and 'jobs' is an API endpoint path (if the path doesn't start with '/' it's assumed to be relative to '/api/v1/'). It handles authentication, retry on failure, parses JSON for you, all that fun stuff. |
|
@AdamWill sure, I remember. Sorry, I did not want to ignore your solution. I just never knew how easy it would be to use it including authentication. @asdil12 https://github.com/os-autoinst/openQA-python-client/blob/master/README.md#usage explains it well |
asdil12
force-pushed
the
bugs
branch
4 times, most recently
from
1908a4b
to
c175ca3
Compare
|
First working version of the feature. |
asdil12
changed the title
|
looks reasonable - but lacks tests |
|
also looks very good to me. Thanks for the integration of the bug icon for closed. I consider it feasible to add a test which makes sure that there is a closed bug icon showing up in the tests overview page, isn't it? |
|
the new controller has 0 test coverage - and api controllers are really easy to cover, so no excuse there. |
|
due to the aweful nature of dbix migrations, you need to rebase |
asdil12
force-pushed
the
bugs
branch
3 times, most recently
from
59cd53c
to
952a22c
Compare
|
Added tests for API and UI and rebased. |
|
+1 |
|
But my tests need some tweaks - so stand by for my next push |
Codecov Report
@@ Coverage Diff @@
## master #1292 +/- ##
=========================================
+ Coverage 87.31% 87.4% +0.08%
=========================================
Files 101 103 +2
Lines 7467 7535 +68
=========================================
+ Hits 6520 6586 +66
- Misses 947 949 +2
Continue to review full report at Codecov.
|
|
Now. |
| # GNU General Public License for more details. | ||
| # | ||
| # You should have received a copy of the GNU General Public License along | ||
| # with this program; if not, write to the Free Software Foundation, Inc., |
There was a problem hiding this comment.
@okurz just replaced all FSF addresses with an URL. Better use this form too
|
|
||
| my $bugs; | ||
| if ($self->param('refreshable')) { | ||
| my $delta = $self->param('delta') ? $self->param('delta') : 3600; |
There was a problem hiding this comment.
do not call param twice - but have the default in a 2nd line or e.g. as
my $delta = $self->param('delta') || 3600;
| is_refreshed => 0, | ||
| t_updated => {'<=' => time2str('%Y-%m-%d %H:%M:%S', time - $delta, 'UTC')} | ||
| }, | ||
| is_existing => 1 |
There was a problem hiding this comment.
these is_ prefices to booleans are rather pointless. Remove them. Especially if 'is existing' is strange grammar - and 'is refreshed' even worse grammar.
| my $ret = {}; | ||
|
|
||
| while (my $w = $bugs->next) { | ||
| next unless ($w->id); |
There was a problem hiding this comment.
how would they not have an id?
|
|
||
| while (my $w = $bugs->next) { | ||
| next unless ($w->id); | ||
| $ret->{$w->id} = $w->bugid; |
There was a problem hiding this comment.
this whole loop sounds like a map { $_->id => $_->bugid } to me
| my ($self) = @_; | ||
|
|
||
| my $bug = $self->db->resultset("Bugs")->find($self->param('bugid')); | ||
| if ($bug) { |
| sub update { | ||
| my ($self) = @_; | ||
|
|
||
| my $bug = $self->db->resultset("Bugs")->find($self->param('bugid')); |
There was a problem hiding this comment.
I suggest to stick with 'id' if you mean an id - also in the WebAPI.pm
t/25-bugs.t
Outdated
| use Test::More; | ||
| use Test::Mojo; | ||
| use Test::Warnings; | ||
| use Test::MockObject; |
There was a problem hiding this comment.
you are not mocking - nor decode json?
lib/OpenQA/WebAPI/Plugin/Helpers.pm
Outdated
| @@ -57,7 +57,12 @@ sub register { | |||
| $app->helper( | |||
| bugicon_for => sub { | |||
| my ($c, $text) = @_; | |||
| return ($text =~ /(poo|gh)#/) ? 'label_bug fa fa-bolt' : 'label_bug fa fa-bug'; | |||
| my $css_class = ($text =~ /(poo|gh)#/) ? 'label_bug fa fa-bolt' : 'label_bug fa fa-bug'; | |||
| my $bug = OpenQA::Schema::Result::Bugs->get_bug($text, $c->db); | |||
There was a problem hiding this comment.
this is heavy. This comes down to a SQL query per icon - in a view. There must be better ways to gather the information right in the controller, where you can also make use of proper joins and caching
t/api/11-bugs.t
Outdated
| use OpenQA::Client; | ||
| use Mojo::IOLoop; | ||
| use Digest::MD5; | ||
| use OpenQA::WebSockets; |
There was a problem hiding this comment.
please only add modules you really use
asdil12
force-pushed
the
bugs
branch
4 times, most recently
from
d149337
to
294ce5c
Compare
See https://progress.opensuse.org/issues/12092