Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow using domains in tenants #5007

Merged
merged 2 commits into from
Dec 13, 2022
Merged

Allow using domains in tenants #5007

merged 2 commits into from
Dec 13, 2022

Conversation

javierm
Copy link
Member

@javierm javierm commented Oct 8, 2022 •
edited

References

Objectives

  • Allow accessing different tenants using different domains and not just subdomains

Visual changes

A group of radio buttons allows indicating whether we're introducing a domain or a subdomain

@javierm javierm self-assigned this Oct 8, 2022
@javierm javierm added this to Doing in Consul Democracy Oct 8, 2022
@javierm javierm mentioned this pull request Oct 8, 2022
Merged
@javierm javierm moved this from Doing to Pending in Consul Democracy Oct 8, 2022
@javierm javierm force-pushed the multitenancy branch 5 times, most recently from 616f69a to 1afdcb2 Compare October 19, 2022 11:10
@javierm javierm force-pushed the multitenancy branch 11 times, most recently from a749cb4 to 4fc0c00 Compare October 25, 2022 13:52
@javierm javierm force-pushed the tenant_variants branch 3 times, most recently from 893be3b to d579bcc Compare November 29, 2022 13:01
Base automatically changed from tenant_variants to master November 29, 2022 13:22
@javierm javierm marked this pull request as ready for review November 30, 2022 18:40
@taitus taitus self-assigned this Dec 2, 2022
@javierm javierm force-pushed the tenant_domains branch 5 times, most recently from 76f9aef to 31b38c3 Compare December 4, 2022 14:36
Consul Democracy automation moved this from Reviewing to Testing Dec 7, 2022
@javierm javierm force-pushed the tenant_domains branch 2 times, most recently from e2a4c9f to ad01f85 Compare December 12, 2022 11:01
@javierm javierm moved this from Testing to Reviewing in Consul Democracy Dec 12, 2022
Consul Democracy automation moved this from Reviewing to Testing Dec 13, 2022
@javierm
Don't create schemas in model tests unless needed
e9c2776 
Creating a schema takes about 3-4 seconds on my machine, so omitting
the callbacks makes tests much faster.

To do so, we're using the `insert!` method added in Rails 6.0, which
inserts a record without executing callbacks or validations. To make the
tests look consistent, we're adding a FactoryBot strategy which uses
`insert!` instead of `create!`.

Note this strategy is useless in most cases because it doesn't work when
models have translatable attributes or associations. However, IMHO it's
worth it even if we only use it for tenants.

We could also use `Tenant.insert!` instead, but then we would have to
add all the mandatory attributes, and in this case the code is clearer
if we only add the attributes we need for the test.
@javierm
Allow having tenants with different domains
e1e16d2 
Some institutions using CONSUL have expressed interest in this feature
since some of their tenants might already have their own domains.

We've considered many options for the user interface to select whether
we're using a subdomain or a domain, like having two separate fields,
using a check box, ... In the end we've chosen radio buttons because
they make it easier to follow a logical sequence: first you decide
whether you're introducing a domain or subdomain, and then you enter it.

We've also considered hiding this option and assuming "if it's got a
dot, it's a domain". However, this wouldn't work with nested subdomains
and it wouldn't work with domains which are simply machine names.

Note that a group of radio buttons (or check boxes) is difficult to
style when the text of the label might expand over more than one line
(as is the case here on small screens); in this case, most solutions
result in the second line of the label appearing immediately under the
radio button, instead of being aligned with the first line of the label.
That's why I've added a container for the input+label combination.
@javierm javierm force-pushed the tenant_domains branch 3 times, most recently from ad01f85 to e1e16d2 Compare December 13, 2022 12:12
@javierm javierm merged commit 09b9d2b into master Dec 13, 2022
Consul Democracy automation moved this from Testing to Release 2.0.0 Dec 13, 2022
@javierm javierm deleted the tenant_domains branch December 13, 2022 12:33
@javierm javierm mentioned this pull request Dec 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions left review comments

@taitus taitus taitus approved these changes

Assignees

@taitus taitus

@javierm javierm

Labels
Community Interest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@javierm @taitus