-
Notifications
You must be signed in to change notification settings - Fork 46
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
Enable multitenancy in test (CI) #989
Conversation
146db52
to
8681179
Compare
spec/requests/home_spec.rb
Outdated
allow(Settings.multitenancy).to receive(:admin_host).and_return('localhost') | ||
end | ||
|
||
context 'with multitenancy', multitenant: true do |
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.
remove multitenant
?
|
||
# There are 3 optional flags available to a test block. Only ONE will be active | ||
# at any given time. They are (with areas of likely use): | ||
# :multitenant - general case default, only needs to be explicit for types described 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.
Can we collapse this to a single setting? tenancy: :single
, tenancy: :multi
, tenancy: :fake
?
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.
If I felt like there were going to be more modes added regularly, I would. That doesn't seem likely.
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.
My concern is that one could set both :multitenant
and :singletenant
and the behavior would be undefined.
Should this all be squashed to a single commit? |
I did a ton of squashing already. There are distinct changes, in particular chronicling enabling the basic setting and then correcting components along the way. It could be any number of commits, but the fewest I would do would be 5. |
This will break, but we should push through to make this the default.
These would only affect Travis.
**:singletenant** This is effectively the reverse of `:multitenant`. It is the cheapest way to fix existing tests that would fail under multitenant default. Singletenant tests - Feature test default singletenant - riiif specs are singletenant - appearances_controller_spec.rb - groups_controller_spec.rb That's how all these were written. **faketenant** Specifically tests that don't care about single or multi-tenancy, they just want there to seem like there is a consistent real tenant. Give all controller tests faketenant unlesss otherwise specified Because the tenant is going to be required for basic routing.
Avoid hardcoding paths in the example.
and tenancy-aware
@jcoyne squashed down to 5 logically distinct commits. |
@jcoyne re-review? |
|
||
# There are 3 optional flags available to a test block. Only ONE will be active | ||
# at any given time. They are (with areas of likely use): | ||
# :multitenant - general case default, only needs to be explicit for types described 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.
My concern is that one could set both :multitenant
and :singletenant
and the behavior would be undefined.
@jcoyne No, the result IS defined, by the conditional. Multitenant would "win". |
Unless anybody wants to concur w/ Justin that reorganizing the flags to be values of single flag is worth blocking this over (I agree w/ him in principle, I just don't think it is worth blocking over), then with due respect, I would proceed on basis of Mike's approval. |
No description provided.