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

Fix Jellyfin not installing correctly #1032

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from

Conversation

KatieTheDev
Copy link

Description

Jellyfin split their repositories into two and have explained why here. To accommodate this change, I have updated the installation script and update script to accommodate their new repository changes.

Fixes issues:

  • Un-tracked issue: Jellyfin stopped installing, giving an error that either a repository was not signed or completely refusing to connect.

Proposed Changes:

  • Changed the installation script to install the latest signing key.
  • Changed the installation script to add the new repository depending on the OS and version the user is running.
  • Changed the update script to remove old signing keys because a previous signing key has expired.
  • Changed the update script to remove old repositories because the repository has changed.

Change Categories

  • Bug fix

Checklist

  • Branch was made off the develop branch and the PR is targetting the develop branch
  • Docs have been made OR are not necessary
    • PR link: Doc changes unnecessary
  • Changes to panel have been made OR are not necessary
    • PR link: Panel changes unnecessary
  • Code is formatted (See more)
  • Code conforms to project structure (See more)
  • Shellcheck isn't screaming (See more)
  • Prints to terminal are handled (See more)
  • I have commented my code, particularly in hard-to-understand areas
  • Testing was done
    • Tests created or no new tests necessary
    • Tests executed

Test scenarios

I tested by running box install and box update using these new scripts. The embedded video was created before I finished work on the update script, but demonstrates how both scripts function. I also did more commits to the install script after making the video, but everything worked anyway as these were mainly stylistic changes or changes to echoes. Test performed on Ubuntu Server 22.04.2 VM in Proxmox VE on a Dell Poweredge R720xd running a fresh install of Swizzin. Only other packages installed on Swizzin were panel and nginx.

If you want to watch the test video in 1080p:
https://youtu.be/Swa9ESljrVU

Uploaded in 480p to meet GitHub size limits:
https://github.com/swizzin/swizzin/assets/51092829/d0f279c7-0c6d-41a6-8207-c5c5addf1726

Architectures

amd64 armhf arm64 Unspecified
Jammy
Focal
Bookworm
Bullseye
Buster
Raspbian ⚫️ ⚫️ ⚫️

✅❎ Passed

🛠🛠 TODO

❌❌ Currently failing

KatieTheDev added 21 commits July 3, 2023 17:10
Jellyfin split their repository into seperate ones for Debian and Ubuntu. These changes will make Swizzin compliant.
I accidentally typed "key" rather than "repository" here in my last commit and didn't notice.
Jellyfin is completely switched to the new repositories. They split Debian and Ubuntu apart. Swizzin should be compliant now.
These variables were not used at all in these scripts.
Update and install should have progress echos.
Made some of the echos more pretty and fitting with the rest of Swizzin.
…nges are made.

Tried installing on amd64 using previous iteration. Failed due to not supporting amd64 even though it is supported.
Previously removed BASE_OS while fixing another bug, accidentally created this one.
Script catches a warning when trying to perform dist_info.
…versions Jellyfin supports and it causes a bug anyway.

Since Swizzin is less compatible than Jellyfin, checking for compatability is redundant. Code removed due to causing program-exiting bugs.
Optimizing key pulling and installation.
…ion of download program selector.

I forgot to include a path for curl and wget.
Previous implementation resulted in key not being grabbed successfully. Works way better if you just assume curl is present.
These echoes are not needed for the script to function as intended.
Unnecessary echo before adding signing key.
Fixing the issues with architecture and distribution filtering that caused issues in the install script. Also made some changes to create better parity between the scripts.
Shellcheck was complaining so trying an experimental fix. Hope this works.
Gets rid of some unreadable code in favor of something that does basically the same thing.
These echoes should fit the overall theme better now.
Jellyfin key used previously has expired. This will automatically pull the new key from Jellyfin.
Copy link
Member

@liaralabs liaralabs left a comment

Choose a reason for hiding this comment

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

Thanks for the PR KatieTheDev. There are a few issues that cropped up during the review that should be resolved, but this otherwise looks good.

Thanks for the contribution!

scripts/install/jellyfin.sh Outdated Show resolved Hide resolved
scripts/install/jellyfin.sh Outdated Show resolved Hide resolved
scripts/install/jellyfin.sh Outdated Show resolved Hide resolved

# Handle some known alternative base OS values with 1-to-1 mappings
# Use the result as the repository base OS
case "${BASE_OS}" in
Copy link
Member

Choose a reason for hiding this comment

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

This case is basically useless aside from setting the _os_codename. If an OS doesn't directly identify itself as Debian or Ubuntu (i.e. during install) then we don't support it.

We certainly don't support linux mint or kde neon

Copy link
Author

Choose a reason for hiding this comment

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

I was unaware we don’t support those distros at all. I will be removing them from the scripts.

Copy link
Author

Choose a reason for hiding this comment

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

I removed unsupported distros, but I am not aware if _os_codename is set as debian on raspbian. It is necessary to make sure raspbian is set to the same repos as debian, as that is what Jellyfin requires.

scripts/install/jellyfin.sh Outdated Show resolved Hide resolved
scripts/update/jellyfin.sh Outdated Show resolved Hide resolved
scripts/update/jellyfin.sh Outdated Show resolved Hide resolved
scripts/update/jellyfin.sh Outdated Show resolved Hide resolved
# Use the result as the repository base OS
ARCHITECTURE="$(dpkg --print-architecture)"
BASE_OS="$(awk -F'=' '/^ID=/{ print $NF }' /etc/os-release)"
case "${BASE_OS}" in
Copy link
Member

Choose a reason for hiding this comment

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

replicated code should be a function; however, this is useless code beyond the setting of the distro codename (as above)

Copy link
Author

Choose a reason for hiding this comment

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

I will use _os_codename in both cases if you can confirm that raspbian is set to debian. If not, I will create a function.


#
# Install the Deb822 format jellyfin.sources entry
echo_progress_start "Adding Jellyfin repository to apt"
Copy link
Member

Choose a reason for hiding this comment

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

Replicated code should be functionized (sources/functions/jellyfin), however I wouldn't block merging based on this, but it would be best-practices for long-term ease of maintenace.

@KatieTheDev
Copy link
Author

I will make the desired changes. Also, I realized that universe was redundant at some point in revising before I submitted the PR, but I must’ve forgotten to either fix that or commit the fix. I also didn’t know about the builtins you mentioned. Also, the reason I disabled force https is because I believe it breaks app support. I will try again with it enabled and see if there’s another workaround for that bug. Something is causing the app not to see the server in the default configuration. I think it may be binding to 127.0.0.1 when nginx is enabled which is blocking outside connections and the apps don’t really like going through nginx. I’ll spend some time on this when I can.

@github-actions github-actions bot added the has conflicts This PR has conflicts against master label Jul 16, 2023
@flying-sausages
Copy link
Member

Also, the reason I disabled force https is because I believe it breaks app support. I will try again with it enabled and see if there’s another workaround for that bug. Something is causing the app not to see the server in the default configuration. I think it may be binding to 127.0.0.1 when nginx is enabled which is blocking outside connections and the apps don’t really like going through nginx. I’ll spend some time on this when I can.

like mobile apps? If that were the case we would have had a lot more people complain about that by now I think

@liaralabs
Copy link
Member

I do think some of the mobile apps won't work with self-signed certificates. The use-case of this self-signed cert is limited only to installs without the nginx proxy though, so the amount of people using self-signed certs on installs without nginx is probably limited. That said, it would also affect folks without a domain/let's encrypt

@KatieTheDev
Copy link
Author

Would it be acceptable for me to add instructions for turning it off in the wiki as a potential fix for app issues?

@flying-sausages
Copy link
Member

Would it be acceptable for me to add instructions for turning it off in the wiki as a potential fix for app issues?

That, plus maybe a echo_docs in the code for when you can see it might be relevant to the users maybe, but not a required addition from my end

@KatieTheDev
Copy link
Author

I don't really know how to fix the conflicts that need to be resolved.

@KatieTheDev
Copy link
Author

I believe that I have resolved merge conflicts and I have merged the latest updates into my branch. I believe I have also resolved the blocking changes, as I have switched everything to builtins that I noticed in your suggestions. I will now work on functionizing things.

@KatieTheDev
Copy link
Author

I am unsure why pre-commit is showing a failure. I have no errors in shellcheck.

@KatieTheDev
Copy link
Author

I added a new commit splitting the repository into a separate function. I was also able to include those new variables into it, as the only place those were used was in that function.

@github-actions github-actions bot removed the has conflicts This PR has conflicts against master label Jul 20, 2023
@@ -40,6 +34,17 @@ if [[ -n $active ]]; then
fi
fi

#
## Get the path to gpg or install it
GNUPG=$(which gpg)
Copy link
Member

Choose a reason for hiding this comment

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

gnupg2 is a global dependency of swizzin. You can safely assume every user has this installed already.

ARCHITECTURE="$(_os_arch)"
BASE_OS="$(_os_distro)"
case "${BASE_OS}" in
raspbian)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think raspbian will pass this check so we don't need to write a case for it

@liaralabs
Copy link
Member

LGTM other than the two comments

@liaralabs
Copy link
Member

I am unsure why pre-commit is showing a failure. I have no errors in shellcheck.

I believe the errors were a result of sausage doing the dev work on pre-commit checks at the same time and the pipeline issues have been resolved. Next time you commit it should re-check 🤞

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

3 participants