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

Make install script compatible with openSUSE #5083

Closed
wants to merge 5 commits into from

Conversation

yubiuser
Copy link
Member

What does this PR aim to accomplish?:

This PRs combines and supersedes #5001 and #5027.
It allows installation of Pi-hole on openSUSE Tumbleweed and Leap(15) and adds the necessary tests.

Link documentation PRs if any are needed to support this PR:

No documentation changes needed. openSUSE is not officially supported, but now compatible with Pi-hole. Uses will still need to bypass OS check during installation


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)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

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

@yubiuser
Copy link
Member Author

The last commit is necessary for the test to pass (permissions for `/etc/lighttpd/lighttpd.conf on openSUSE prevent 'other' from reading it). I thought it might be a good solo-PR to always set the permissions so I created #5038 which can be merged separately if wanted.

@yubiuser yubiuser added the PR: Approval Required Open Pull Request, needs approval label Dec 22, 2022
@yubiuser yubiuser requested a review from a team December 22, 2022 12:58
@yubiuser yubiuser force-pushed the openSUSE3 branch 5 times, most recently from 69d9a41 to 82ade5f Compare December 29, 2022 20:51
@PromoFaux
Copy link
Member

It allows installation of Pi-hole on openSUSE Tumbleweed and Leap(15) and adds the necessary tests.

No documentation changes needed. openSUSE is not officially supported, but now compatible with Pi-hole. Uses will still need to bypass OS check during installation

Some questions and thoughts:

If we don't officially support it, why then are we adding in special handling and tests for this OS? If we're adding in special handling and tests for this OS, why don't we officially support it?

Why are we also including tests against the rolling release version (and by extension accepting version=any)? We don't do this for Debian Testing...

I would like to hear from @dschaper and @DL6ER on this, too.

In theory I have nothing against increasing compatibility - but we either support it or we don't. This reads like some strange in-between land :)

@PromoFaux PromoFaux requested review from DL6ER and dschaper and removed request for a team January 22, 2023 16:07
@DL6ER
Copy link
Member

DL6ER commented Jan 22, 2023

Sorry, I have been so busy with FTL development in every free minute that I haven't actually looked at the other repos recently - so I missed this one, too. Having tests doesn't feel right to me. I see why we should have them (because all features should be tested as much as is maintainable) but I'm more thinking about the consequences of "what will happen when it breaks". This kind of forces us to fix SUSE when it breaks. Or we'll have to live with red crosses on all commits from this point in time on. Or we have to rip out the code we added here and remove the tests. I guess you see my line of argumentation.

TL;DR: Tests forcing us to fix things make it a supported OS. A supported OS that nobody is running at home (or is familiar enough to quickly fix quirks) is not really a supported OS. Sounds very much like an insoluble problem.

@yubiuser
Copy link
Member Author

yubiuser commented Jan 22, 2023

Why are we also including tests against the rolling release version (and by extension accepting version=any)?

Because the original author (coogor) targeted tumbleweed in the first place. I included any to have a way to "allow any version" (set by the resp. DNS TXT record) because the version set in /etc/os-release will change very frequently even in the images we pull for the tests. As this is a rolling release distro it didn't made much sense to set a fixed version here.

__

TL;DR: Tests forcing us to fix things make it a supported OS. A supported OS that nobody is running at home (or is familiar enough to quickly fix quirks) is not really a supported OS. Sounds very much like an insoluble problem.

When I saw the contribution from coogor I thought that with only a small change to basic-install.sh we could offer to run Pi-hole on a so far completely new distro. But I also needed a way to make sure it will really run on that distro - so I added the tests. I know it's a strange in-between-land. We could also drop the test and just make the installer compatible and in case it's breaking we wait for issues/PRs from users actually running openSUSE...

@rdwebdesign
Copy link
Member

rdwebdesign commented Jan 22, 2023

I'm not against supporting a new OS, but I'm against the "in-between-land" state. We support, or we don't officially support.

I think removing the tests and use the strategy "fix when it's broken" (only if it's possible to fix) is the way to go, since there are no developers using this OS.

@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label Jan 22, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

yubiuser and others added 2 commits January 22, 2023 21:24
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: coogor <axel.braun@gmx.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
@github-actions github-actions bot removed the PR: Merge Conflict Issue blocking check and merge of code label Jan 22, 2023
@github-actions
Copy link

Conflicts have been resolved.

@dschaper
Copy link
Member

It is kind of a strange situation.

I don't mind the tests directly, it's nice to know if things work on other operating systems. However, what do we do when SUSE is the only broken OS? Do we set the SUSE tests to be non-required? How do we determine when to allow the test to fail and when to require the PR code to be changed for SUSE.

I don't like the idea of testing against, or even supporting, a rolling release. Way too many variables to account for.

I guess the determining factor for me is 3. I am willing to help maintain this change if there are issues with it later. If you are willing to make sure that the changes in this PR are going to be maintained by you then I'm okay with it.

@yubiuser
Copy link
Member Author

yubiuser commented Feb 2, 2023

I removed Tumbleweed tests after internal discussion.

Signed-off-by: Christian König <ckoenig@posteo.de>
@coogor
Copy link

coogor commented Feb 20, 2023

I removed Tumbleweed tests after internal discussion.

So only testing for Leap or are you intending to bin support for openSUSE (not SUSE) completely?

@yubiuser
Copy link
Member Author

yubiuser commented Feb 20, 2023

We decided that we don't have the resources to "support" a rolling distribution which can break anytime without us making changes to Pi-hole. Therefore we won't implement tests for Tumbleweed. The code itself should still work. Tests for Leap are also still part of this PR.

@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label Apr 19, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: Christian König <ckoenig@posteo.de>
@github-actions github-actions bot removed the PR: Merge Conflict Issue blocking check and merge of code label Apr 20, 2023
@github-actions
Copy link

Conflicts have been resolved.

@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label May 28, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link

Conflicts have been resolved.

@github-actions github-actions bot removed the PR: Merge Conflict Issue blocking check and merge of code label May 28, 2023
Signed-off-by: Christian König <ckoenig@posteo.de>
@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label Feb 4, 2024
Copy link

github-actions bot commented Feb 4, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the stale label Mar 6, 2024
Copy link

github-actions bot commented Mar 6, 2024

Existing merge conflicts have not been addressed. This PR is considered abandoned.

@github-actions github-actions bot closed this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Approval Required Open Pull Request, needs approval PR: Merge Conflict Issue blocking check and merge of code stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants