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

Add maintenance-mode command #153

Closed
yorkshire-pudding opened this issue Jun 25, 2021 · 11 comments · Fixed by #232
Closed

Add maintenance-mode command #153

yorkshire-pudding opened this issue Jun 25, 2021 · 11 comments · Fixed by #232
Assignees

Comments

@yorkshire-pudding
Copy link
Collaborator

On Drupal sites, using I would include the maintenance mode activation at the beginning and deactivation at the end of my update process. I think a simple command to do this would be good. I can see 2 possible avenues:

  1. a dedicated command just for setting maintenance mode to true or false
  2. a state-set and state-get command set like config-set and config-get if there are other states that it would be useful to be able to set using a bee command.

I have found a workaround to do this:

bee eval "state_set('maintenance_mode',TRUE);"
bee eval "state_set('maintenance_mode',FALSE);"

but I think a command would be better.

@ghost ghost added the type: enhancement label Jun 25, 2021
@serundeputy
Copy link
Member

@yorkshire-pudding

Adding commands:

  • bee state-get <STATE>
  • bee state-set <STATE> <VALUE>
  • bee maintenance-mode <VALUE>

image


PR: #174

@ghost ghost added the pr: needs review label Feb 25, 2022
@ghost
Copy link

ghost commented Feb 25, 2022

@serundeputy Thanks for the PR! I posted a review. Sorry in advance for being so pedantic...

@ghost ghost added pr: needs work and removed pr: needs review labels Feb 25, 2022
@ghost ghost mentioned this issue Apr 27, 2022
@yorkshire-pudding
Copy link
Collaborator Author

yorkshire-pudding commented Oct 21, 2022

I'm working on addressing the feedback that @BWPanda raised on PR #174
During testing, I encountered some intermittent issues that related to the fact that a Backdrop fresh install has no maintenance_mode state but also around the different types being pushed.
I found this issue on the backdrop-issues queue that makes it clear that maintenance_mode should always be boolean so I've changed that but someone using bee state-set maintenance_mode could inadvertently set it wrong currently.

I've also done a bit to handle what is returned in state-get according to the type of value stored.

I'd like to understand what people might use state-set for given there is a dedicated maintenance mode function. One idea I have is to check the current type and try to ensure that any state-set commands provide the right type; this might also involve preventing any update of array types to reduce complexity.

I note that @jenlampton said on #186 that:

I'd love to use this for backdrop as well. (It's particularly useful when working on the maintenance page)".

Please could you elaborate?

@indigoxela @klonos @BWPanda @quicksketch - do you have any thoughts about states, whether in core or from contrib that it would be useful to be able to set via bee state-set?

Another thing I wondered for state-get is where there should be a --raw-value argument that returns the value (for use in a function) without any surrounding message. Thoughts?

@indigoxela
Copy link
Member

do you have any thoughts about states, whether in core or from contrib that it would be useful to be able to set via bee state-set?

Currently the maintenance_mode is the only useful state to set I'm aware of. There might be contrib states... not sure.

@quicksketch
Copy link
Member

Yeah, same as @indigoxela. Maintenance mode seems by far the most useful one, but I could see some potential ones:

  • database_utf8mb4_active: This could be set if you know your tables are UTF8MB4 even if Backdrop doesn't know.
  • node_cron_last: This could be set if you're doing development and want to test your cron hooks. Modules also sometimes provide their own last cron states that might be helpful to set (like redirect_cron_last).

@yorkshire-pudding
Copy link
Collaborator Author

Big thanks to @serundeputy for the initial work, thanks to @BWPanda for the feedback on that which has informed this and thanks for feedback from @indigoxela and @quicksketch .

This builds on earlier work but ensures the type of each state is respected, and also presents each state type in a sensible format. Only integers, booleans and strings can be set with state-set and the maintenance-mode enforces a boolean. Boolean states can be set with either true/false (case insensitve) or 1/0 but are displayed as TRUE/FALSE.

I'd be grateful for any testing and feedback.

@indigoxela
Copy link
Member

It works! 👍

I tested with maintenance-mode (the command), state-get, state-set (with maintenance_mode, and update_last_check...).

The error messages when one does something wrong (like typos) are a bit misleading sometimes.

And I'm not really sure if we need two commands that do (mostly) the same thing (bee maintenance-mode vs. bee state-set mainentance_mode). But @yorkshire-pudding had some good arguments. 😉

So my conclusion is: it works, but UX might get improved in follow-ups.

@yorkshire-pudding
Copy link
Collaborator Author

@indigoxela - thanks for your feedback in office hours earlier today.
I've tried to improve the messaging if the state doesn't exist or if the wrong type of value has been entered. I've also tried to improve the help text and removed whitespace from the test file.

@indigoxela
Copy link
Member

I've tried to improve the messaging...

@yorkshire-pudding this is great! Much easier to work with now. I tried with missing and wrong arguments and the messages really help now to do things properly. 👍

@yorkshire-pudding
Copy link
Collaborator Author

Thanks @indigoxela and @klonos
Let me know if any more feedback. If none, I will squash merge 😉 .

yorkshire-pudding added a commit that referenced this issue Nov 7, 2022
@yorkshire-pudding
Copy link
Collaborator Author

Thanks to @serundeputy, @BWPanda, @indigoxela , @klonos, @quicksketch , @jenlampton . This is now merged.

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

Successfully merging a pull request may close this issue.

4 participants