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

Implement occ maintenance:mode --check-if-disabled #35830

Closed
wants to merge 1 commit into from
Closed

Implement occ maintenance:mode --check-if-disabled #35830

wants to merge 1 commit into from

Conversation

leegarrett
Copy link

@leegarrett leegarrett commented Dec 19, 2022 •
edited by szaimen
Loading

Signed-off-by: Lee Garrett [email protected]

Summary

Add a parameter that can be used in scripts. Returns 0 when there is no maintenance mode, and 1 if there is.

TODO

  • Test coverage?

Checklist

@szaimen szaimen added enhancement 3. to review Waiting for reviews labels Dec 19, 2022
@szaimen szaimen added this to the Nextcloud 26 milestone Dec 19, 2022
@szaimen szaimen requested review from a team, ArtificialOwl, skjnldsv and CarlSchwan and removed request for a team December 19, 2022 22:30
@leegarrett leegarrett mentioned this pull request Dec 20, 2022
Closed
skjnldsv
skjnldsv requested changes Dec 20, 2022
View reviewed changes
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the working confusing
Maintenance mode is not "disabled", it's either on or off.
The meaning is different

@leegarrett
Copy link
Author

I find the working confusing Maintenance mode is not "disabled", it's either on or off. The meaning is different

I'm open for a different wording. --check-if-off? --is-off?

@leegarrett
Copy link
Author

I just used the wording from similar calls, btw:

www-data@nextcloud:~/nextcloud$ php ./occ maintenance:mode
Maintenance mode is currently disabled

@MichaIng
Copy link
Member

I like the short variant --is-disabled or --is-off. Not sure whether "disabled" is so bad, not only as it's used by CLI output as well, but also I personally use it. But I'm no native English speaker.

Also probably --is-enabled/--is-on is a little more intuitive, so true is "enabled"/"on"?

@leegarrett
Copy link
Author

I like the short variant --is-disabled or --is-off. Not sure whether "disabled" is so bad, not only as it's used by CLI output as well, but also I personally use it. But I'm no native English speaker.

Also probably --is-enabled/--is-on is a little more intuitive, so true is "enabled"/"on"?

I'd also avoid negations is settings/options. Maybe --is-normal-mode? No idea what the opposite of maintenance mode is called in nextcloud terminology.

@JojoBr0
Copy link

JojoBr0 commented Dec 20, 2022

I find the working confusing Maintenance mode is not "disabled", it's either on or off. The meaning is different

I'm open for a different wording. --check-if-off? --is-off?

How about --status ?

0 means maintenance:mode is off
1 means maintenance:mode is on

@skjnldsv
Copy link
Member

--status

Seems like a good idea? What do you all prefer?
If you all are ok with --check-if-disabled, I don't mind that much, and we can merge as is :)

@MichaIng
Copy link
Member

MichaIng commented Dec 21, 2022 •
edited
Loading

I fear that with --status some would need to regularly lookup or test whether true is on or off 😄, hence prefer a self-explaining one in this regards.

I'm personally for --is-on, short, no negation and it's clear that true means "on". And in the same turn or dedicated PR, some CLI and web UI outputs can be aligned so that maintenance mode is either "on" or "off" instead of "enabled"/"disabled", following --on/--off as well.

@leegarrett
Copy link
Author

After some pondering I'm also throwing --is-up into the ring. This avoids any negation, and refers to "up", as opposed to being "down for maintenance". What do y'all think about this?

@skjnldsv
Copy link
Member

This avoids any negation, and refers to "up"

So, the server is up for maintenance? 🙈
Sorry, couldn't resist. I'll see myself out

@skjnldsv
Copy link
Member

I'm personally for --is-on, short, no negation and it's clear that true means "on". And in the same turn or dedicated PR, some CLI and web UI outputs can be aligned so that maintenance mode is either "on" or "off" instead of "enabled"/"disabled", following --on/--off as well.

I second that, occ maintenance:mode --is-on is straightforward and self explanatory

@MichaIng
Copy link
Member

MichaIng commented Dec 22, 2022 •
edited
Loading

So, the server is up for maintenance? 🙈

The same possible confusion came to my mind: IMHO, it should be as clear as possible whether on/up refers to Nextcloud (up == maintenance off) or the maintenance mode (up/on == Nextcloud down). Since currently maintenance:mode command names and outputs all refer to the maintenance mode (on/enabled == Nextcloud down), that should be kept, at best re-using the same wording (on/off instead of up/down) to rule out any confusion.

@leegarrett
Copy link
Author

How about ./occ status --rc-only? 0 for running fine, 1 for maintance mode, and we have 2-254 for any other future conditions that could mean degraded operation.

@leegarrett leegarrett changed the title Implement occ maintenance:mode --check-if-disabled Implement occ status command via return codes Dec 22, 2022
leegarrett pushed a commit to leegarrett/server that referenced this pull request Dec 22, 2022
Implement occ status command via return codes (Fixes: nextcloud#35830)
e7b514c 
Running `./occ status -e` will write any output. However, it will:

exit 0 during normal operation,
exit 1 when in maintenance mode,
exit 2 when `./occ upgrade` is needed.
leegarrett pushed a commit to leegarrett/server that referenced this pull request Dec 22, 2022
Implement occ status command via return codes (Fixes: nextcloud#35830)
dae64e9 
Running `./occ status -e` will write any output. However, it will:

exit 0 during normal operation,
exit 1 when in maintenance mode,
exit 2 when `./occ upgrade` is needed.

Signed-off-by: Lee Garrett <[email protected]>
@leegarrett leegarrett changed the title Implement occ status command via return codes Implement occ maintenance:mode --check-if-disabled Dec 22, 2022
@leegarrett leegarrett mentioned this pull request Dec 22, 2022
Merged
7 tasks
@skjnldsv
Copy link
Member

occ maintenance:mode --is-on

Different goals and discussion

Can you adjust this PR to occ maintenance:mode --is-on please? :)

@skjnldsv
Copy link
Member

Ah, I see #35873

They serve different purpose IMHO, so I would keep both (one is api-oriented, while this one is human-oriented)

@leegarrett
Copy link
Author

Ah, I see #35873

They serve different purpose IMHO, so I would keep both (one is api-oriented, while this one is human-oriented)

Both PRs are intended to be used by machines, not humans. I retract this PR over #35873, as the latter is more expandable in the future.

@leegarrett leegarrett closed this Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skjnldsv skjnldsv skjnldsv requested changes

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.

Implement occ status command via return codes
6 participants
@leegarrett @MichaIng @JojoBr0 @skjnldsv @CarlSchwan @szaimen