Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more query parameters for configuring last builds #575

Merged
merged 3 commits into from Feb 29, 2016

Conversation

@okurz
Copy link
Member

@okurz okurz commented Feb 25, 2016

  • Add optional query parameters to configure builds limit
  • t: Delete dead code
  • Add optional query parameters for showing previous builds
@@ -24,7 +24,9 @@ use OpenQA::Schema::Result::Jobs;
sub _group_result {
my ($self, $group, $limit) = @_;

my $timecond = {">" => time2str('%Y-%m-%d %H:%M:%S', time - 24 * 3600 * 14, 'UTC')};
my $time_limit_days = $self->param('time_limit_days') // 14;
Copy link
Member

@aaannz aaannz Feb 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it, param validation is something we are long overdue. Would Mojo recovers when you supply i.e. text instead of \d+?

Loading

Copy link
Member Author

@okurz okurz Feb 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes you can really think around corners and ask about weird unusual cases. I like that :-)

Calling http://localhost:9526/group_overview/1?time_limit_days=foo renders an empty page and the log output:

[00][Thu Feb 25 11:44:16 2016] [debug] GET "/group_overview/1"
[00][Thu Feb 25 11:44:16 2016] [debug] Routing to controller "OpenQA::WebAPI::Controller::Main" and action "group_overview"
[00][Thu Feb 25 11:44:16 2016] [debug] Retrieving results for up to 10 builds up to foo days old
[00]Argument "foo" isn't numeric in multiplication (*) at
[00]    /home/okurz/local/os-autoinst/openQA/script/../lib/OpenQA/WebAPI/Controller/Main.pm line 29 (#1)
[00]    (W numeric) The indicated string was fed as an argument to an operator
[00]    that expected a numeric value instead.  If you're fortunate the message
[00]    will identify which operator was so unfortunate.
[00]    
[00][Thu Feb 25 11:44:16 2016] [debug] Rendering cached template "main/group_overview.html.ep"
[00][Thu Feb 25 11:44:16 2016] [debug] Rendering cached template "layouts/info.html.ep"
[00][Thu Feb 25 11:44:16 2016] [debug] Rendering cached template "main/group_builds.html.ep"
[00][Thu Feb 25 11:44:16 2016] [debug] Rendering cached template "layouts/bootstrap.html.ep"
[00][Thu Feb 25 11:44:16 2016] [debug] Rendering cached template "branding/openSUSE/navbar.html.ep"
[00][Thu Feb 25 11:44:16 2016] [debug] 200 OK (0.014780s, 67.659/s)

So I would not over-engineer something here. As long as it is only used as a URL parameter I would not really invest more time in checking this more, an empty page should be good enough. If we would provide something like a search field this would invite also "unexperienced" users to type something and we should catch bad user input. agreed?

Loading

Copy link
Member

@aaannz aaannz Feb 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I was only curious. Sometimes when script dies in Mojo controller it doesn't run that code anymore until restarted. I was worried being it this problem, but looks like it survived.

Loading

@aaannz
Copy link
Member

@aaannz aaannz commented Feb 25, 2016

LGTM

Loading

@okurz
Copy link
Member Author

@okurz okurz commented Feb 25, 2016

Loading

okurz added 3 commits Feb 28, 2016
The index dashboard and group overview page limit the number of previous
builds based on hardcoded numbers. Adding query parameters this is easily
adjustable e.g. if one wants to get an overview of a longer history.

Example to show up to three week old builds instead of the default two weeks
with up to 20 builds instead of up to 10 being the default for the group
overview page:

    http://openqa/group_overview/1?time_limit_days=21&limit_builds=20
$driver has "find_element(<tag>, 'css')" which is doing the equivalent of
"find_element_by_css(<tag>)" so why keep the "TODO" around?
Replace hardcoded number of 500 with the optional query parameter with keeping
500 as a default.

Higher numbers would yield more complex database queries but can be selected
for special investigation use cases with the advanced query parameters.
@okurz okurz force-pushed the feature/more_query_overwrite branch from 8113924 to 850d535 Feb 28, 2016
aaannz pushed a commit that referenced this issue Feb 29, 2016
Add more query parameters for configuring last builds
@aaannz aaannz merged commit b71c1d8 into os-autoinst:master Feb 29, 2016
1 check passed
Loading
@okurz okurz deleted the feature/more_query_overwrite branch Feb 29, 2016
@aaannz
Copy link
Member

@aaannz aaannz commented Feb 29, 2016

Just as a future note: later I'm for to add limit to $limit. I have a feeling we are making DDOSes way too easy this way.

Loading

@okurz
Copy link
Member Author

@okurz okurz commented Feb 29, 2016

exactly the thing I am working on right now :-)

Loading

@okurz
Copy link
Member Author

@okurz okurz commented Feb 29, 2016

ok, not exactly what I am working on as I don't know what a good value would be

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants