-
Notifications
You must be signed in to change notification settings - Fork 21
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 root problem list for objects with problem and are part of dependency #1057
base: dependencies
Are you sure you want to change the base?
Conversation
bc06e1c
to
3375e51
Compare
4351639
to
e77c687
Compare
e77c687
to
a2fb6b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rebase, #1055 is merged.
$filter = Filter::equal('id', $this->item->id); | ||
$relations = [ | ||
'from', | ||
'from.to.host', | ||
'from.to.host.state', | ||
'from.to.service', | ||
'from.to.service.state' | ||
]; | ||
|
||
$summary = RedundancyGroupParentStateSummary::on($this->getDb()) | ||
->with($relations) | ||
->filter($filter); | ||
|
||
$members = RedundancyGroup::on($this->getDb()) | ||
->columns([ | ||
'id' => 'id', | ||
'parent_output' => new Expression( | ||
'CASE WHEN redundancy_group_from_to_host_state.output IS NULL' | ||
. ' THEN redundancy_group_from_to_service_state.output' | ||
. ' ELSE redundancy_group_from_to_host_state.output END' | ||
), | ||
'parent_long_output' => new Expression( | ||
'CASE WHEN redundancy_group_from_to_host_state.long_output IS NULL' | ||
. ' THEN redundancy_group_from_to_service_state.long_output' | ||
. ' ELSE redundancy_group_from_to_host_state.long_output END' | ||
), | ||
'parent_checkcommand_name' => new Expression( | ||
'CASE WHEN redundancy_group_from_to_host.checkcommand_name IS NULL' | ||
. ' THEN redundancy_group_from_to_service.checkcommand_name' | ||
. ' ELSE redundancy_group_from_to_host.checkcommand_name END' | ||
), | ||
'parent_last_state_change' => new Expression( | ||
'CASE WHEN redundancy_group_from_to_host_state.last_state_change IS NULL' | ||
. ' THEN redundancy_group_from_to_service_state.last_state_change' | ||
. ' ELSE redundancy_group_from_to_host_state.last_state_change END' | ||
), | ||
'parent_severity' => new Expression( | ||
'CASE WHEN redundancy_group_from_to_host_state.severity IS NULL' | ||
. ' THEN redundancy_group_from_to_service_state.severity' | ||
. ' ELSE redundancy_group_from_to_host_state.severity END' | ||
) | ||
]) | ||
->with($relations) | ||
->filter($filter) | ||
->orderBy([ | ||
'parent_severity', | ||
'parent_last_state_change', | ||
], 'DESC'); | ||
|
||
$this->applyRestrictions($members); | ||
|
||
/** @var RedundancyGroup $data */ | ||
$data = $members->first(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite like that this is evaluated per group result.
Keep it as it is for now, but update it so that it matches the changes made to the statistics and badges implementation.
I have an alternative implementation in mind, but that's now far to complex for this change and so will be implemented separately. Prior approval of this change, I will request to remove this to not show any badges or output for groups. Until then, it's kept to test the statistics and badges implementation changes.
b41bdb3
to
18db2a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add PhpDoc and method return type to newly introduced classes/methods.
d797382
to
01bf509
Compare
243c129
to
8782937
Compare
489d077
to
6b52b63
Compare
2c12a58
to
98fc692
Compare
The state text is only necessary to add css rules for state-balls and not be used to show the state text of the group.
38a3596
to
8e95d30
Compare
…ted objects tooltip
… `RedundancyGroupListItem`
8e95d30
to
061f67a
Compare
return null; | ||
} | ||
|
||
$path = 'from.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the trailing dot.
|
||
$path = 'from.'; | ||
$subQueryRelation = $relation !== null ? $relation . $path : $path; | ||
$subQuery = $this->query->createSubQuery(new DependencyEdge(), $subQueryRelation, null, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not a linked subquery? This is for selecting a column, not for a filter. The latter usually benefits from not linking as there are way more rows involved. Selected columns are only required for a small subset of rows, so a linked query is presumably better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw that the sub query is linked anyway, since you filter after service.id
manually below. Problem with this is, it's hardcoded and ignores $relation
at all, while just a line below it's considered.. Please clean this up.
$subQuery->getSelectBase() | ||
->join( | ||
['to_dependency_node' => 'dependency_node'], | ||
["to_dependency_node.id = $subQueryAlias.to_node_id"] | ||
)->joinLeft( | ||
['root_dependency' => 'dependency'], | ||
[ "$subQueryAlias.dependency_id = root_dependency.id"] | ||
)->joinLeft( | ||
['root_dependency_state' => 'dependency_state'], | ||
['root_dependency.id = root_dependency_state.dependency_id'] | ||
)->joinLeft( | ||
['root_group' => 'redundancy_group'], | ||
['root_group.id = to_dependency_node.redundancy_group_id'] | ||
)->joinLeft( | ||
['root_group_state' => 'redundancy_group_state'], | ||
['root_group_state.redundancy_group_id = root_group.id'] | ||
)->where( | ||
new Expression("root_dependency_state.failed = 'y' OR root_group_state.failed = 'y'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why you perform the joins on your own here. Shouldn't a filter for parent.redundancy_group.state.failed=y|dependency.state.failed=y
on the query suffice?
use ipl\Sql\Select; | ||
|
||
/** | ||
* Redundancy group's summary (The nodes could only host and service) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The nodes could only host and service)
I know why you added this, but 99% of other readers don't. So for them this only states the obvious. Please remove.
// For PostgreSQL, ALL non-aggregate SELECT columns must appear in the GROUP BY clause: | ||
if ($q->getDb()->getAdapter() instanceof Pgsql) { | ||
/** | ||
* Ignore Expressions, i.e. aggregate functions {@see getColumns()}, | ||
* which do not need to be added to the GROUP BY. | ||
*/ | ||
$candidates = array_filter($select->getColumns(), 'is_string'); | ||
// Remove already considered columns for the GROUP BY, i.e. the primary key. | ||
$candidates = array_diff_assoc($candidates, $groupBy); | ||
$groupBy = array_merge($groupBy, $candidates); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is, dead code. Above ->columns
is used. So there's no chance that any other columns are selected. On the other hand, those that are selected are all expressions.
} | ||
} | ||
|
||
.state-badge { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is specific to the dependency-node-state-badges widget, right? Then please add this to its less file.
justify-content: space-between; | ||
|
||
.plugin-output { | ||
.line-clamp(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a comment, explaining that this is necessary due to the caption not being a -webkit-box anymore
.redundancy-group-list-item { | ||
.caption { | ||
display: flex; | ||
justify-content: space-between; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, and because you already have a rule for plugin output, why not making the latter flex: 1 1 auto
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent element main
has display: block
property, flex: 1 1 auto
does not work. And changing the display
property in main
to flex
could have other consequences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flex: 1 1 auto
on .plugin-output
of course.
|
||
.object-statistics { | ||
ul { | ||
display: flex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, this should be applied for dependency-node-state-badges
to make all the state badges appear in a single row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are in a single row, all state badges are. For me this rule didn't have any effect, which is why I'm asking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I should have been clear, this happens when you have both handled and unhandled state badges. And now that I have tested it again, it should be applied to the dependency-node-state-badges
and all ul
elements in the dependency-node-state-badges
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I'm wondering why this needs to be specifically handled for dependency node state badges. They are not the only badges with handled/unhandled states which are shown in a single line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is caused by display: flex
on caption.
} | ||
|
||
.state-badges { | ||
font-size: 0.75em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes badges illegible.
ref #1050
blocked by: #1055
blocked by: Icinga/ipl-web#231