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

Change OS Detection in debug script #3434

Merged
merged 2 commits into from Jun 5, 2020
Merged

Change OS Detection in debug script #3434

merged 2 commits into from Jun 5, 2020

Conversation

PromoFaux
Copy link
Member

@PromoFaux PromoFaux commented May 24, 2020

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

What does this PR aim to accomplish?:

Currently the supported/unsupported OS detection is based on the distro name being one of 5 set values, this is fine, but then we also need to check further. For example Raspbian <9 is not supported.

This PR adds that check. I'd like to get a similar output into the installer if it fails, however I am starting to think that maybe the installer should prompt user to run and read the debug log if an update fails. Though obviously that won't necessarily work on a fresh install.

Examples using a Debian 10 (Buster) system:
supportedos.txt contains:

Raspbian=9,10
Ubuntu=16,18
Debian=9,10
Fedora=31,32
CentOS=7,8

Debug output:
image

supportedos.txt contains:

Raspbian=9,10
Ubuntu=16,18
Debian=9
Fedora=31,32
CentOS=7,8

Debug Output:
image

supportedos.txt contains:

Raspbian=9,10
Ubuntu=16,18
Fedora=31,32
CentOS=7,8

Debug Output:
image

@PromoFaux PromoFaux requested review from dschaper, DL6ER and a team May 24, 2020 14:36
@PromoFaux PromoFaux force-pushed the new/os_detect branch 2 times, most recently from 7db9cbf to 3cca306 Compare May 24, 2020 15:20
@yubiuser
Copy link
Member

From you example

Debian=9
Distro is supported but version is not - please upgrade

Wording doesn't make sense if version > supported version

@PromoFaux
Copy link
Member Author

PromoFaux commented May 24, 2020

Debian=9

The intention of this is not to say "Version 9 and up are supported", but rather "only version 9 is supported", hence it fails the check when the detected version is 10

Obviously in real life we support Debian 10, but this is just an demonstration of what the output would look like if the detected version is not listed.

edit: Never mind, I think I misunderstood what you wrote :)

double-edit: I've updated the screenshots on the OP

@yubiuser
Copy link
Member

Better but not perfect ;-)
You could check if ${os_version} is bigger or smaller than the biggest/smallest supported version and change the error accordingly (not yet supported/not supported - please upgrade)

@PromoFaux
Copy link
Member Author

That's not too bad an idea. I'll have a think on it later

@PromoFaux
Copy link
Member Author

I may change up some of the logic in this PR, actually, based on #3441

@PromoFaux PromoFaux added this to the v5.1 milestone Jun 3, 2020
…ript

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
advanced/Scripts/piholeDebug.sh Outdated Show resolved Hide resolved
advanced/Scripts/piholeDebug.sh Outdated Show resolved Hide resolved
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@dschaper dschaper merged commit 5f9dac8 into development Jun 5, 2020
@dschaper dschaper deleted the new/os_detect branch June 5, 2020 20:38
@PromoFaux PromoFaux mentioned this pull request Jul 5, 2020
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-5-1-released/35577/1

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

Successfully merging this pull request may close these issues.

None yet

4 participants