nextcloud / serverinfo

📊 A monitoring app which creates a server info dashboard for admins
GNU Affero General Public License v3.0
95 stars 61 forks source link

feat: rename DefaultOs to Linux #488

Closed kesselb closed 1 year ago

kesselb commented 1 year ago

DefaultOs was actually the implementation for Linux.

MichaIng commented 1 year ago

So depending on the detected OS, the app tries to gather system info via common commands/APIs. And the DefaultOs is basically a dummy where no external commands/APIs are used?

Definitely better than before with Linux actually named Linux and a universally compatible fallback/default OS, which additionally can be used when external commands/APis are explicitly not wanted or available. But I find the name "default" not very intuitive code-wise and for the CLI/setting.

What about calling it DummyOs or SkeletonOs or something which better expresses what it actually is? Not sure if there are cases where PHP_OS can be wrong, but for this, it could be useful to have the setting changed to e.g. enforce_os_type which then takes linux, freebsd or dummy to maintain the same effect force_default_os currently has. Otherwise, probably rename it to disable_system_calls or something like that, which better explains what it is for without the need to read the README?

joshtrichards commented 1 year ago

Suggestion:

kesselb commented 1 year ago

Thank you very much for your feedback :raised_hands:

I couldn't come up with a better name, so I stayed with DefaultOs :rofl:

Not sure if there are cases where PHP_OS can be wrong

Afaik PHP_OS is set on compilation: https://github.com/php/php-src/blob/6f6fedcb46a27cd3530f0babc9b03ce4598f9eab/configure.ac#L1498-L1499

(and might also help reduce some of the work needed to be made by forkers that I've seen where hosters basically tear our a lot of the OS stuff)

Good to know, I wasn't aware.

MichaIng commented 1 year ago

Afaik PHP_OS is set on compilation

Indeed, no point then to allow enforcing anything else than the dummy.

Two concurrently written feedback comments with the same point(s) about the naming 💯. "dummy", "skeleton", "base", "basic", "bare", "minimal" ... "fallback"? If it acted like a template, "base" would fit best. But here it is an independent class which just implements the same interface, so "dummy" or "minimal" probably fit better.

The setting could then be called force_dummy_os or force_minimal_os, but it does not necessarily need to contain the class name? I like the word "restricted" in this context, so restrict_external_calls or restricted_mode or something like that would be another idea.

kesselb commented 1 year ago

Thanks for your valuable input :+1: