DependencyGraph: switch "parent" and "child" terminology #10263
Conversation
The .ti files call `DependencyGraph::AddDependency(this, service.get())`. Obviously, `service.get()` is the parent and `this` (Downtime, Notification, ...) is the child. The DependencyGraph terminology should reflect this not to confuse its future users.
Al2Klimov
force-pushed
the
DependencyGraph-parent-child
branch
from
5547c12 to
188ba53
Compare
|
|
Because, in wise foresight, I've done this work even before you requested it. And yes, you requested to "simply fix the terminology" which I've done here – no less no more. (If not "in an adapted way/differently",) This would look strange to me. Then it lands again, via #10000, on your to-review list anyway.
But ok. Let's first hear @yhabteab's opinion on this PR. |
| @@ -520,7 +520,7 @@ String ScriptUtils::MsiGetComponentPathShim(const String& component) | |||
|
|
|||
| Array::Ptr ScriptUtils::TrackParents(const Object::Ptr& child) | |||
| { | |||
| return Array::FromVector(DependencyGraph::GetParents(child)); | |||
| return Array::FromVector(DependencyGraph::GetChildren(child)); | |||
There was a problem hiding this comment.
The method is called TrackParents and not children, how is this supposed to work now?
<4> => get_object(Dependency, "brother!mother")
{
__name = "brother!mother"
active = true
child_host_name = "brother"
child_service_name = ""
disable_checks = false
disable_notifications = true
....
<5> => track_parents(get_object(Dependency, "brother!mother"))
[ ]
<8> => track_parents(get_host("mother"))
[ {
__name = "brother!mother"
active = true
child_host_name = "brother"
child_service_name = ""
disable_checks = false
disable_notifications = true
...There was a problem hiding this comment.
Then it has never worked, because I literally just renamed methods and method args.
There was a problem hiding this comment.
If we want to also fix this, it would be a breaking change. (I'd even drop the method, so that its users notice easily.) But this is something for v2.15, i.e another PR. (And no, I won't open another PR right now, don't worry.)
There was a problem hiding this comment.
If @julianbrost wants to merge this while that DSL function is still broken (was broken before too), then it would probably work as expected with #10000like for the first time since it was introduced :).
There was a problem hiding this comment.
That function looks like a early debugging left-over to me. It's not mentioned in the documentation and from the name track_parents it isn't really clear what it does.
(I'd even drop the method, so that its users notice easily.) But this is something for v2.15
Sounds like what I would do. (If we figure out that there's a real-world use for it, it should probably get a proper name and documentation.)
There was a problem hiding this comment.
And the best discovery tool for real-world usages is removal.😈
There was a problem hiding this comment.
But yes, your fact checking is correct:
$ git grep -nFie track_parents
lib/base/scriptutils.cpp:52:REGISTER_SAFE_FUNCTION(System, track_parents, &ScriptUtils::TrackParents, "child");
$ git grep -nFie track\\_parents
$ git grep -nFie track\\\\_parents
$
Same at Google btw.: https://letmegooglethat.com/?q=%22icinga%22+2+%22track_parents%22
There was a problem hiding this comment.
Yay 🎉! It works now with d28dfec!
<11> => track_parents(get_object(Dependency, "brother!mother"))
[ {
__name = "mother"
acknowledgement = 0.000000
acknowledgement_expiry = 0.000000
acknowledgement_last_change = 0.000000
action_url = ""
active = true
address = ""
address6 = ""
check_attempt = 1.000000
check_command = "dummy"
check_interval = 60.000000
check_period = ""
check_timeout = null
command_endpoint = ""
display_name = "mother"
downtime_depth = 0.000000
enable_active_checks = true
enable_event_handler = true
enable_flapping = false
enable_notifications = true
enable_passive_checks = true
....
There was a problem hiding this comment.
Obviously,
service.get()is the parent andthis(Downtime, Notification, ...) is the child.
For the calls of DependencyGraph::AddDependency() that are done explicitly in source files from the repo.
There are also the usages generated by mkclass:
icinga2/tools/mkclass/classcompiler.cpp
Lines 902 to 952 in e50eb52
These are DependencyGraph::RemoveDependency(this, /* other config object that's referenced in the configuration of this */), so this is the child, like it is now with the fixed names inside DependencyGraph.
The .ti files call
DependencyGraph::AddDependency(this, service.get()). Obviously,service.get()is the parent andthis(Downtime, Notification, ...) is the child. The DependencyGraph terminology should reflect this not to confuse its future users.