-
Notifications
You must be signed in to change notification settings - Fork 428
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
Multi tenancy pm #3075
Multi tenancy pm #3075
Conversation
Codecov Report
@@ Coverage Diff @@
## multi-tenancy-phase-1 #3075 +/- ##
=========================================================
+ Coverage 78.09% 78.70% +0.60%
=========================================================
Files 374 387 +13
Lines 31219 31658 +439
=========================================================
+ Hits 24382 24916 +534
+ Misses 6837 6742 -95
Continue to review full report at Codecov.
|
hide_service_name configuration for specific host type (previously for specific domain) make no sense, since we don't know the domain name at the moment when we fetch the value of the option. in fact (before introduction of the host types) we were always using the value for ?MYNAME host, which was the first item in the 'hosts' list.
0e069ca
to
17c0c9a
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.
Looks good, I have a few specific comments. Also, as we agreed, there is an urgent need to fix the use of the ?MYNAME
macro that is now doing something else than expected in some places in the code.
%% they have dummy values). | ||
%% DummyDomainName value must be unique to avoid accidental config keys removal. | ||
DummyDomainName = <<"dummy.domain.name">>, | ||
M = maybe_insert_dummy_domain(M0, DummyDomainName), |
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 idea of putting ?HOST
here was to create a valid config. Now the config is invalid in some tests as dummy.domain.name
is not the same host as in host_config
(see eq_host_config
). So the tests now show (and expect) that an invalid config is parsed correctly... This might be confusing.
I'd rather keep a valid host name here and fix the issue later than have a wrong value set on purpose.
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.
eq_host_config/2
explicitly uses ?HOST
value in both checks.
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 point is that after the change some test cases are parsing inconsistent configuration, e.g.
#{<<"hosts">> => [<<"dummy...">>], <<"host_config">> => [{<<"host_type">> => ?HOST, ....}]}
Such configuration does not make sense and maybe it even should be considered a an error.
However, valid or not, it is not typical and I'd rather have typical setups in the test.
I think it's a minor issue and it does not block merging.
this PR introduces:
mongoose_router_dynamic_domains
that is responcible for a lazy adding (on the first routing attempt) of dynamic domains. removal of dynamic domains happens synchronously ondisable_domain
hook.default_server_domain
- it must be used for reporting stream errors before user clarifies the domain name he wants to use (before user sets up the stream).