Navigation Menu

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

Show evaluation errors #569

Closed
wants to merge 2 commits into from
Closed

Show evaluation errors #569

wants to merge 2 commits into from

Conversation

kquick
Copy link
Contributor

@kquick kquick commented Jun 29, 2018

Display evaluation errors on the project summary page and also on the Status:Latest Evaluations page.

In the original form, processing of a jobset can be affected by evaluation errors, but there is no indication of an evaluation error unless you visit the specific jobset page. This modification makes these evaluation errors visible on top-level summaries (in the same places that build failures are shown) so that there is visible indication of a problem that should be fixed.

If there are no errors, there should be no difference in the display from the current output. This only shows additional information when there are evaluation errors.

@@ -341,10 +341,20 @@ sub evals :Local Args(0) {

my $evals = $c->model('DB::JobsetEvals');

my @failEvals = [$c->model('DB::Jobsets')
->search({ "me.enabled" => 1, "me.hidden" => 0,
Copy link
Member

Choose a reason for hiding this comment

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

There are some indentation issues here.
Also, it may be better to do only one search call, as it is implemented in the search method below (while I'm really not sure about this:/).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure quite what you mean about indentation "issues". There are multiple dereferences, and each reference is somewhat long, so this separates each dereference to a separate line. The code works just fine (I run it locally). Was your concern about functionality or just appearance, and did you have a specific recommendation regarding appearance?

In my experience it is typical for ORMs to perform local optimization of the generated SQL statement, and multiple refinements like this are often recommended as a way to specialize a base query. Additionally the SQL server performs optimization on the query, so I don't expect this will result in any performance problems, especially when used with postgres.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, forget my comment about indentation and thx for your answer!

@grahamc
Copy link
Member

grahamc commented Mar 18, 2019

I feel a bit scared about adding this to the load. These pages already do pretty heavy pageloads on hydra.nixos.org -- adding more, heavy queries seem spooky.

@nlewo
Copy link
Member

nlewo commented May 3, 2019

@grahamc Actually, one commit (e7a7bd5) of this PR should not increase the load and is quite nice (it shows evaluation errors from the project view).
I'm using this commit on my Hydra instance and it is really useful.
What about merging this commit?

Regarding the second commit, if we really want this feature, we could add a configuration variable to enable this feature. It would be disable by default and people wanting this feature could explicitly enable it.

@edolstra
Copy link
Member

edolstra commented May 6, 2019

Thanks, I've cherry-picked commit e7a7bd5.

@gilligan
Copy link
Contributor

Considering the concerns on the one hand, and the fact that parts were cherry picked i’ll close this PR.

Thank you for the contribution.

@gilligan gilligan closed this May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants