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

basic-install with openSUSE support #5001

Closed
wants to merge 1 commit into from

Conversation

coogor
Copy link

@coogor coogor commented Nov 5, 2022

replacing PR #5000 against master

Signed-off-by: coogor axel.braun@gmx.de

Thank you for your contribution to the Pi-hole Community!

Please read the comments below to help us consider your Pull Request.

We are all volunteers and completing the process outlined will help us review your commits quicker.

Please make sure you

  1. Base your code and PRs against the repositories developmental branch.
  2. Sign Off all commits as we enforce the DCO for all contributions

  • What does this PR aim to accomplish?:

This PR adds support for openSUSE as discussed in #4947

  • How does this PR accomplish the above?:

It adds determination for zypper as package manager. It adds packages (naming) used in openSUSE to achieve proper installation of the pihole package. Is uses systemd-service (as soon as available)

  • What documentation changes (if any) are needed to support this PR?:

Comments and messages with relation to openSUSE are added to the installation script.
Installer was tested on openSUSE Tumbleweed and MicroOS


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

  • I have read the above and my PR is ready for review. Check this box to confirm

@dschaper
Copy link
Member

dschaper commented Nov 5, 2022

Thank you, could you please fill out the template for this PR. The template will be the documentation we refer to if this PR is merged and having the information there is very helpful when we need to refer back to what this PR was meant to do.

@coogor
Copy link
Author

coogor commented Nov 6, 2022

**What does this PR aim to accomplish?:**

This PR adds support for openSUSE as discussed in #4947

**How does this PR accomplish the above?:**

It adds determination for zypper as package manager. It adds packages (naming) used in openSUSE to achieve proper installation of the pihole package. Is uses systemd-service (s soon as available)

**What documentation changes (if any) are needed to support this PR?:**

Comments and messages with relation to openSUSE are added to the installation script.
Installer was tested on openSUSE Tumbleweed and MicroOS

@coogor
Copy link
Author

coogor commented Nov 6, 2022

Thank you, could you please fill out the template for this PR. The template will be the documentation we refer to if this PR is merged and having the information there is very helpful when we need to refer back to what this PR was meant to do.

Template was filled, let me know if you need adidtional information.
I have seen some check report failure (trailing whitespace) - Can I somehow update the submission/pull request or do I need to create a new one? Sry, not familiar with github!

@yubiuser
Copy link
Member

yubiuser commented Nov 6, 2022

You can make your modifications locally, commit them locally and push to your fork on github. This PR will automatically update.

@rdwebdesign
Copy link
Member

@coogor

Template was filled, let me know if you need adidtional information.

You need to Edit the PR text.
Use the options menu, select Edit and change the original post.

image

automated install/basic-install.sh Outdated Show resolved Hide resolved
@coogor
Copy link
Author

coogor commented Nov 7, 2022

Edited the description and added double quotes

Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

The whole section L361 - L405 has a lot of duplicated code. Maybe you could separate what they OS have in common and only apply a if [ OS = XX] when really needed.

automated install/basic-install.sh Outdated Show resolved Hide resolved
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

I refactored your code a bit to reduce doubling. What do you think?

      # These variable names match the ones for apt-get. See above for an explanation of what they are for.
      PKG_INSTALL=("${PKG_MANAGER}" install -y)
      OS_CHECK_DEPS=(grep bind-utils)
      LIGHTTPD_USER="lighttpd"
      LIGHTTPD_GROUP="lighttpd"
      LIGHTTPD_CFG="lighttpd.conf.fedora"
        
        if [ "${PKG_MANAGER}" = "dnf" ] || [ "${PKG_MANAGER}" = "yum" ]  then
          # CentOS package manager returns 100 when there are packages to update so we need to || true to prevent the script from exiting.
          PKG_COUNT="${PKG_MANAGER} check-update | egrep '(.i686|.x86|.noarch|.arm|.src)' | wc -l || true"
          INSTALLER_DEPS=(git dialog iproute newt procps-ng which chkconfig ca-certificates)
          PIHOLE_DEPS=(cronie curl findutils sudo unzip libidn2 psmisc libcap nmap-ncat jq)
          PIHOLE_WEB_DEPS=(lighttpd lighttpd-fastcgi php-common php-cli php-pdo php-xml php-json php-intl)

        # If the host OS is centos (or a derivative), epel is required for lighttpd
          if ! grep -qiE 'fedora|fedberry' /etc/redhat-release; then
              if rpm -qa | grep -qi 'epel'; then
                  printf "  %b EPEL repository already installed\\n" "${TICK}"
              else
                  local RH_RELEASE EPEL_PKG
                  # EPEL not already installed, add it based on the release version
                  RH_RELEASE=$(grep -oP '(?<= )[0-9]+(?=\.?)' /etc/redhat-release)
                  EPEL_PKG="https://dl.fedoraproject.org/pub/epel/epel-release-latest-${RH_RELEASE}.noarch.rpm"
                  printf "  %b Enabling EPEL package repository (https://fedoraproject.org/wiki/EPEL)\\n" "${INFO}"
                  "${PKG_INSTALL[@]}" "${EPEL_PKG}"
                  printf "  %b Installed %s\\n" "${TICK}" "${EPEL_PKG}"
              fi
          fi
        else
          # openSUSE packages
          # openSUSE package manager returns number when there are packages to update so we need to || true to prevent the script from exiting.
          PKG_COUNT="${PKG_MANAGER} lu | egrep '(.i686|.x86|.noarch|.arm|.src)' | wc -l || true"
          INSTALLER_DEPS=(git dialog iproute newt procps which ca-certificates)
          PIHOLE_DEPS=(cronie curl findutils sudo unzip libidn2 psmisc libcap-ng0 nmap ncat jq)
          PIHOLE_WEB_DEPS=(lighttpd php8 php8-fastcgi php8-cli php8-pdo php8-intl php8-openssl php8-sqlite)

@yubiuser yubiuser mentioned this pull request Nov 18, 2022
@yubiuser
Copy link
Member

I setup a test environment for openSUSE Thumbleweed to be able to test your PR. The test will pull the latest image of opensuse/tumbleweed and run our test suite against your changes.

As you can see, it fails at some tests:
https://github.com/pi-hole/pi-hole/actions/runs/3500340861/jobs/5862908143

To continue, please provide the output of cat /etc/os-release for the current thumbleweed

@yubiuser
Copy link
Member

Also: please rebase on latest development and sovle the merge conflicts.

@coogor
Copy link
Author

coogor commented Nov 19, 2022

Thank you, I wanted to have a look into this on the weekend, but dont mind you were faster :-)
Rebasing...looks like this does not work automatically. 'Classic' approach with editor I guess?

@yubiuser
Copy link
Member

Yes, you need to rebase locally and force push afterwards.

@coogor
Copy link
Author

coogor commented Nov 19, 2022

As you can see, it fails at some tests: https://github.com/pi-hole/pi-hole/actions/runs/3500340861/jobs/5862908143

To continue, please provide the output of cat /etc/os-release for the current thumbleweed

docb@X1E:~/buildservice> cat /etc/os-release 
NAME="openSUSE Tumbleweed"
# VERSION="20221115"
ID="opensuse-tumbleweed"
ID_LIKE="opensuse suse"
VERSION_ID="20221115"
PRETTY_NAME="openSUSE Tumbleweed"
ANSI_COLOR="0;32"
CPE_NAME="cpe:/o:opensuse:tumbleweed:20221115"
BUG_REPORT_URL="https://bugs.opensuse.org"
HOME_URL="https://www.opensuse.org/"
DOCUMENTATION_URL="https://en.opensuse.org/Portal:Tumbleweed"
LOGO="distributor-logo-Tumbleweed"

@yubiuser
Copy link
Member

Tumbleweed, as a rolling distribution, has only a latest image we could test against. A often changing image is not a good foundation for tests. I will change the test image therefore to Leap 15.4.

@coogor
Copy link
Author

coogor commented Nov 19, 2022

oops, why was that closed.
rebase has happened, against original extentions.
No trying to apply refactored code

@yubiuser
Copy link
Member

yubiuser commented Nov 20, 2022

Tests are currently failing at

/opt/pihole/basic-install.sh: line 2284: which: command not found\n').stdout

However, we don't use which at that place anymore:

lib=$(ldd "$(command -v sh)" | grep -E '^\s*/lib' | awk '{ print $1 }')

This was changed in #4907
__

This makes me think, something might went wrong with your rebase. Did you pull the development branch before rebasing?

@yubiuser
Copy link
Member

yubiuser commented Nov 20, 2022

This is what your PR reverrts:

lib=$(ldd "$(which sh)" | grep -E '^\s*/lib' | awk '{ print $1 }')

@coogor
Copy link
Author

coogor commented Nov 20, 2022

Did you pull the development branch before rebasing?
Yes.....

That makes me think, something might went wrong with your rebase. Did you pull the development branch before rebasing?

I Think the commit line from this file mentioned something of 'which' in it.
What do you propose to continue? Sorry for causing work, this is the first time I really work on github

@yubiuser
Copy link
Member

Sorry for causing work, this is the first time I really work on github

No worries. We all started somewhere and made the same mistakes. We are here to help.

What do you propose to continue?

You would need to refresh (pull) the latest development changes from pi-hole into your local fork and rebase again. This might cause merge conflicts now.
If you feel like this is to much trouble (but bonus: you learn something about git), you could start the PR from scratch (not a big deal, you can basically copy&paste) your changes as they all happen in one file so far. However, as I'm working on the test suite in parallel, you would need to update your development branch and rebase anyway, as we would merge the test suite first and then this PR (to really test this PR before merging).

@coogor
Copy link
Author

coogor commented Nov 21, 2022

What do you propose to continue?

You would need to refresh (pull) the latest development changes from pi-hole into your local fork and rebase again. This might cause merge conflicts now.

No...it says I'm on the latest stage: This branch is not behind the upstream pi-hole:development (3 commits ahead)

@coogor
Copy link
Author

coogor commented Nov 22, 2022

Tested the latest version of installation script successfully up to the point where pihole-ftl.service should be invoked.
AFAIK the proper systemd-service is not in the master branch yet?

@yubiuser
Copy link
Member

No it's not. I'm working on the test suite, but it needs some more work

@yubiuser
Copy link
Member

Tests are passing now.

https://github.com/pi-hole/pi-hole/actions/runs/3527352033/jobs/5938401382

However, I discovered a few places in basic-install that also need changes (because there are conditions we do not capture atm, which where revealed by testing Leap 15). I'll open some PRs (think of 3-4) to fix them. When they all are merged, you need to rebase your PR on development again to be able to pass the Leap test suite.

@coogor
Copy link
Author

coogor commented Nov 24, 2022

Tests are passing now.

https://github.com/pi-hole/pi-hole/actions/runs/3527352033/jobs/5938401382

good. However, I feel Tumbleweed would be the better test target, as it is the basis for MicroOS (small, transactional, self-maintaining...) and this is IMO the best solution for a local DNS server...

However, I discovered a few places in basic-install that also need changes (because there are conditions we do not capture atm, which where revealed by testing Leap 15). I'll open some PRs (think of 3-4) to fix them. When they all are merged, you need to rebase your PR on development again to be able to pass the Leap test suite.

OK, let me know once done. I had just synced the devel branch and that was OK. Or create the PR directly against my fork :-)

@rdwebdesign
Copy link
Member

Tumbleweed is a rolling release and we probably will only support stable releases.

@yubiuser
Copy link
Member

yubiuser commented Nov 24, 2022

These PRs need to be merged before

#5037
#5038
#5039

automated install/basic-install.sh Outdated Show resolved Hide resolved
automated install/basic-install.sh Outdated Show resolved Hide resolved
automated install/basic-install.sh Outdated Show resolved Hide resolved
@yubiuser
Copy link
Member

yubiuser commented Nov 25, 2022

Test on tumbleweed fail until #4924 is merged too. Adding this to the test suit and adjusting the test:
https://github.com/pi-hole/pi-hole/actions/runs/3550794823

(It fails for tumbleweed because it's not an supported OS, all other tests pass)

@yubiuser
Copy link
Member

Tumbleweed passes now as well: https://github.com/pi-hole/pi-hole/actions/runs/3553020655/jobs/5977810768

@yubiuser yubiuser linked an issue Dec 4, 2022 that may be closed by this pull request
Signed-off-by: coogor <axel.braun@gmx.de>
@coogor
Copy link
Author

coogor commented Dec 19, 2022

every time I sync with devel, this gets closed

@coogor coogor reopened this Dec 19, 2022
@@ -1385,6 +1395,11 @@ installConfigs() {
if [[ -d '/run/systemd/system' ]]; then
install -T -m 0644 "${PI_HOLE_LOCAL_REPO}/advanced/Templates/pihole-FTL.systemd" '/etc/systemd/system/pihole-FTL.service'

# Set net admin permissions so that FTL can serve DNS, DHCP and IMAP (for DHCPv6). If this does not work, run FTL as root user.
Copy link
Member

Choose a reason for hiding this comment

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

Did you re-add this intentionally? It was removed by #5043

@coogor
Copy link
Author

coogor commented Dec 21, 2022 via email

@yubiuser
Copy link
Member

yubiuser commented Dec 22, 2022

I cherry-picked your commit (to preserve your authorship) and removed the additional lines. I also combined it with my PR to implement the necessary test suites. See #5083

@coogor
Copy link
Author

coogor commented Dec 22, 2022

Thanks! looks like go-live gets closer :-)

@coogor
Copy link
Author

coogor commented Jan 7, 2023

can we close this pull request?

@dschaper dschaper closed this Jan 7, 2023
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.

Installer dies silently on openSUSE
4 participants