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.

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

[4.x] Add command to bring the tenants up and down from maintenance and remove deprecated exception #761

Merged
merged 24 commits into from
Sep 29, 2022

Conversation

stein-j
Copy link
Contributor

@stein-j stein-j commented Dec 2, 2021

This PR changes the maintenance mode to comply to the changes made in Laravel 8.

New

  • Adds two CLI commands to the package: tenancy:up and tenancy:down with the same parameters as the default Laravel command except for the option render because it is not possible to render a page before Laravel is booted for a specific tenant.

  • Adds the function bringUpFromMaintenance in the trait MaintenanceMode.

Changes

A tenant in maintenance mode now throws an HttpException exception instead of the deprecated MaintenanceModeException exception.

Closes #533.

@stein-j stein-j changed the title [3.x] Add function to bring the tenant up from maintenance [3.x] Add command to bring the tenants up and down from maintenance Dec 3, 2021
@stancl
Copy link
Member

stancl commented Dec 6, 2021

Hey. This looks good, thanks for the contribution! I think I had some maintenance mode related tasks written in my backlog for v4, so I'll probably merge this when working on the feature. Should be in January I think.

@stancl stancl added the v4 label Dec 6, 2021
@stein-j stein-j changed the title [3.x] Add command to bring the tenants up and down from maintenance [4.x] Add command to bring the tenants up and down from maintenance Dec 6, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 25, 2021

Codecov Report

Merging #761 (7875fbd) into 3.x (20e1fa1) will decrease coverage by 1.39%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                3.x     #761      +/-   ##
============================================
- Coverage     87.02%   85.62%   -1.40%     
- Complexity      372      375       +3     
============================================
  Files           103      105       +2     
  Lines          1102     1120      +18     
============================================
  Hits            959      959              
- Misses          143      161      +18     
Impacted Files Coverage Δ
src/Commands/Down.php 0.00% <0.00%> (ø)
src/Commands/Up.php 0.00% <0.00%> (ø)
src/Database/Concerns/MaintenanceMode.php 75.00% <0.00%> (-25.00%) ⬇️
src/TenancyServiceProvider.php 96.66% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20e1fa1...7875fbd. Read the comment docs.

src/Commands/Down.php Outdated Show resolved Hide resolved
@stancl
Copy link
Member

stancl commented Dec 25, 2021

Closing #757 and will finish that here. We could probably add --secret but I'm not sure if there's a point in having a different secret than the central app.

@stein-j
Copy link
Contributor Author

stein-j commented Dec 25, 2021

Laravel now simply throws a HttpException exception. https://github.com/laravel/framework/blob/8f3b411969ffdd733a0ba54098e08f8ca7c1369e/src/Illuminate/Foundation/Http/Middleware/PreventRequestsDuringMaintenance.php#L78

If you give me a few days, I can update the code and comply to the new maintenance mode used by Laravel and then update the commands so that we can use the same parameters as Laravel.
I think all parameters can be recreated except for render because this allows to return a response even before Laravel is initialised (which means, before the package is also initialised).

Note: Naturally this would allow #757

@stancl
Copy link
Member

stancl commented Dec 25, 2021

Sure, thanks a lot for the work on this! FWIW if there's a signature change, you can use the version in Laravel 9. And probably change the target to the master branch.

@stein-j
Copy link
Contributor Author

stein-j commented Dec 26, 2021

I'm glade you mentioned Laravel 9 because coincidently two PRs (1, 2) have been accepted during those last few days and it changed the maintenance system.

L9 will now have a configurable driver for the maintenance storage system (which will determine how and where the data is stored), at the time of writing this comment there are two drivers/managers file: Which is the old way to put the application to maintenance ; cache : Which will store the data inside the cache.

The reason they added drivers, is because when using a load balancer and you set the application into maintenance mode it would only work on the application the command has been typed on... so a cache driver was needed. But this package doesn't need such implementation because even if you would use a load balancer, there should be only one database connection per tenant for all applications (at least in most use cases).

So I think to keep things simple, this PR will keep its own feature and keep storing the data in the database. It might be important to mention in the documentation that the maintenance mode doesn't uses the driver from the config file.

Maybe in another PR we can try to create our custom driver, tenant compatible.

@stancl
Copy link
Member

stancl commented Dec 26, 2021

this PR will keep its own feature and keep storing the data in the database

That is in the data column in our case, right?

@stein-j
Copy link
Contributor Author

stein-j commented Dec 26, 2021

Yes

@stein-j stein-j changed the base branch from 3.x to master December 27, 2021 12:02
@stein-j stein-j changed the title [4.x] Add command to bring the tenants up and down from maintenance [4.x] Add command to bring the tenants up and down from maintenance and remove deprecated exception Jan 6, 2022
@stein-j stein-j requested a review from stancl January 6, 2022 15:46
@stancl
Copy link
Member

stancl commented Jan 6, 2022

Just a note: the tests will fail because master has some failing tests due to dependencies that need new syntax. But we can look at this feature's tests in isolation

@stein-j
Copy link
Contributor Author

stein-j commented Feb 20, 2022

@stancl the PR is done and all the tests have been added.
As you said there are conflict versions with composer, so the tests fails. Do you want me to look into it ? Or should we wait that the master to be updated ?

@stancl
Copy link
Member

stancl commented Feb 20, 2022

Probably wait until master is updated 👍🏻

When we add L9 support, I'll merge 3.x into master

@stein-j stein-j changed the base branch from master to 3.x March 8, 2022 13:23
@stancl
Copy link
Member

stancl commented Jul 27, 2022

@lukinovec I think we should finish this PR before the pending tenants since it's smaller and it interacts with the trait adding a --tenants option. That way we'll make the conflict resolution simpler.

Also, the tests are showing as failing, but I think they'll run differently when the CI is triggered again since I simplified the CI matrix in the master branch. So if you could resolve the conflicts, that would trigger a CI build and we'll see how the feature works now.

@stancl
Copy link
Member

stancl commented Aug 1, 2022

Only php-cs-fixer is failing, tests seem to be passing, so CI-wise this PR is ready now.

I'll look at the code now and see if this is ready to be merged.

src/Commands/Down.php Outdated Show resolved Hide resolved
@stancl stancl marked this pull request as ready for review August 1, 2022 15:58
@stancl
Copy link
Member

stancl commented Aug 1, 2022

@stein-j Is anything unimplemented yet or is the PR fully ready? Is the code made to work with Laravel 9 (the changes you made to the way tenants are put into maintenance mode)?

@stein-j
Copy link
Contributor Author

stein-j commented Aug 1, 2022

@stancl I want to go over it again, it's been a while and a file was changed in another PR since (https://github.com/archtechx/tenancy/pull/802/files#diff-40487242039e9842dc87ba96285282e42d0cc5b9a7dbeac6f115c4ee2bb8db59). Let me get back to you in a few hours.

@stein-j
Copy link
Contributor Author

stein-j commented Aug 1, 2022

@stancl what will be the minimum requirements for Laravel and PHP for V4 ?

@stein-j stein-j marked this pull request as draft August 1, 2022 17:27
@stancl
Copy link
Member

stancl commented Aug 1, 2022

Just that the syntax is updated for Laravel 9. I think you already did this since the PR is recent. But I'm just checking in case anything is missing.

@stein-j
Copy link
Contributor Author

stein-j commented Aug 1, 2022

Ok, it's all good.

@stein-j stein-j marked this pull request as ready for review August 1, 2022 21:37
@stancl
Copy link
Member

stancl commented Sep 29, 2022

FYI I merged master into this so you should delete the 3.x branch after we merge this since it isn't compatible with our 3.x (in case you plan on making PRs to 3.x in the future)

@stancl stancl removed the incomplete label Sep 29, 2022
@stancl stancl merged commit 121370e into archtechx:master Sep 29, 2022
@stancl
Copy link
Member

stancl commented Sep 29, 2022

Thanks for your work on this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Disabling maintenance mode, and CLI commands for maintenance mode
4 participants