-
Notifications
You must be signed in to change notification settings - Fork 429
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
adding dynamic domains support to mod_ping #3136
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3136 +/- ##
==========================================
+ Coverage 79.46% 79.48% +0.02%
==========================================
Files 395 395
Lines 32196 32197 +1
==========================================
+ Hits 25584 25592 +8
+ Misses 6612 6605 -7
Continue to review full report at Codecov.
|
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 think it's pretty much ready to merge, I just have some minor comments in the meantime 👌🏽
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.
Lokks good in general, but I have a few comments.
big_tests/test.config
Outdated
@@ -18,6 +18,7 @@ | |||
%% so that we rein the "bag of things" approach | |||
{hosts, [{mim, [{node, mongooseim@localhost}, | |||
{domain, <<"localhost">>}, | |||
{host_type, <<"localhost">>}, |
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 isn't necessary, see below.
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 would rather have it defined like that, not as rpc-s.
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.
For me it's better to have one helper that consistently reads it from mongoose_domain_core
instead of defining it multiple times for multiple hosts. Anyway, as the helper is already there, let's use it consistently. This way we can change the implementation later when needed.
…:host_type/1 interface
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.
Looks good. The only thing that could be done would be the removal of host type from test.config
.
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.
Excellent work 👌🏽
This PR introduces dynamic domains support for mod_ping.
Service discovery doesn't work for dynamic domains (as mod_disco doesn't support it yet), but still works correctly for statically configured domains
for a local testing you can use these commands: