Grouping Health Checks – squashed commits #123
Conversation
|
@DrSchimke ah ok thx .. you can actually force push the same branch to just update your existing PR. |
| $runner = $this->getContainer()->get('liip_monitor.runner'); | ||
| $group = $input->getOption('group'); | ||
|
|
||
| if (is_null($group)) { |
There was a problem hiding this comment.
symfony syle is to use null === $group instead
…tests comming soon)
DrSchimke
force-pushed
the
grouping-squashed
branch
4 times, most recently
from
1371126
to
5e8be23
Compare
|
Hi Lukas, please have a look. We made some changes, regarding your review comments. |
| $checkCollectionServices = $container->findTaggedServiceIds('liip_monitor.check_collection'); | ||
|
|
||
| $groups = array($defaultGroup); | ||
| $groups = array_merge($groups, $this->getGroups($checkServices)); |
There was a problem hiding this comment.
array_merge takes any number of parameters http://php.net/manual/en/function.array-merge.php
There was a problem hiding this comment.
I know, but I like this
$groups = array($defaultGroup);
$groups = array_merge($groups, $this->getGroups($checkServices));
$groups = array_merge($groups, $this->getGroups($checkCollectionServices));
$groups = array_merge($groups, $this->getGroupsFromParameter($container));more than that:
$groups = array_merge(
array($defaultGroup),
$this->getGroups($checkServices),
$this->getGroups($checkCollectionServices),
$this->getGroupsFromParameter($container)
);For me, it is better readable: a bit shorter (in terms of line count) and these four quite uniform statements are simpler than one large statement, containing more parts.
Ok, in the end, the difference is quite small. say, and I will change it.
There was a problem hiding this comment.
I find the later more readable because I have just one function name to "parse" when reading.
|
This all looks great to me .. the main thing still left is to explain some where (in the code or dev docs section in the README ..) this |
|
In Postcon@5e8be23#diff-3f5582b5992ee90b21e0ad051df6c184R56 we tried to explain this thing. |
DrSchimke
force-pushed
the
grouping-squashed
branch
from
5e8be23
to
7eeebb4
Compare
|
@kbond do you have some feedback or all good from your side? |
|
Haven't looked at this yet but I will in the next day or so. |
| * | ||
| * @return null|Runner | ||
| */ | ||
| public function getRunner($group) |
There was a problem hiding this comment.
so my last open question is if we should allow null here as a parameter and in that case use the default group
|
I tested this PR on a project and all looks good. Fully BC and the group feature works well. A few suggestions for the
|
… pr liip#115 – contains changes by @arosslau @puhbooh and @DrSchimke
DrSchimke
force-pushed
the
grouping-squashed
branch
from
7eeebb4
to
65ddc12
Compare
|
Hi Kevin, we like your suggestions for the HealthCheckCommand, but would prefer to merge the current version before implementing these additional features. After this merge, you, we or someone else can make a new PR. Best regards |
|
sounds good to me .. so lets merge? good for you @kbond ? |
|
Sounds good, merge away!
|
Grouping Health Checks – squashed commits
|
wow .. this is so awesome! thanks @DrSchimke ! |
|
oops .. I got a bit too excited and released things right away .. |
…or all checks with monitor:health
…or all checks with monitor:health
This is an alternative PR for issue #113.
In original PR #115 it was recommended to put all changes into one commit. So, here it is...