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.

Sign up for GitHub

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

Jump to bottom

Add Jellyfin as an option - Testing #72

Closed
wants to merge 1 commit into from
Closed

Add Jellyfin as an option - Testing #72

wants to merge 1 commit into from

Conversation

umbertix
Copy link
Contributor

This resolves: #44

I still need to run some more test, but would like to open the discussion and see if there is something else that you might want for this :)

Thanks for the hard work already done.

@kubealex
Copy link
Owner

hi @umbertix and thank you for your PR!
I'm definitely interested in reviewing this one, and it seems everything is in place for it to be considered good, I only have a question, does it need any config tuning to be included as an init container like it happens for all the other components? (authentication, URL path to be configured or similar?)

Thank you!!

@umbertix
Copy link
Contributor Author

I haven't got time to look at it yet sorry, but I do imagine that will need some more work, this was just to open the dicsussion about the feature, since there is some interest I will try look into it a bit more.

@umbertix
Copy link
Contributor Author

I've tested the changes and this will succesfully spin up a working Jelly fin instance. The user will still will have to do a couple of manual steps but I think it's a good starting point.

@kubealex
Copy link
Owner

Thank you so much for your contribution @umbertix!
I will give it a run during the weekend, I wanted to ask you what manual steps are required, is it something that we can address with an init container as for other tools?

@umbertix
Copy link
Contributor Author

I was checking the manual steps, and what is the result for the config files. I haven't seen anything in the documentation that seems to point at a headless config option. And the XML config file looks rather ugly.
Some of the steps are user/password setup and initial folders setup. I think is a nice addition but not super necessary to get things running.
I'm still playing around with it to see if I can automate the user/password creation but it doesn't look too promissing as of right now.

@kubealex
Copy link
Owner

Super, I think I will be able to run it today, if the initial config is something that is jellyfin related (like Plex that asks you to choose folders and similar or creating a user) I think we can keep it outside the definition.

Prowlarr has something similar introduced recently, but we usually take care of what's related to the deployment rather than the actual user experience (https/context/storage) leaving users freedom to customize the rest.

In the meantime we integrated some changes @InputObject2 worked on, it would be great if you can pull them into your branch, since many changes affect the tool definition in the CR, I will be happy to support injecting them in the Jellyfin bits as well.

Thank you for the contribution, looking forward to having it integrated soon 🚀

@reedjosh
Copy link

reedjosh commented Dec 5, 2023

Just want to add a plus one to this PR.

I personally deployed Jellyfin from @umbertix's fork and it's working great for me!

@reedjosh reedjosh mentioned this pull request Dec 30, 2023
Open
@kubealex kubealex changed the title Add Jellyfin as an option Add Jellyfin as an option - Testing Jan 3, 2024
@kubealex
Copy link
Owner

kubealex commented Jan 3, 2024

As I promised yesterday, here I am :)
Soooo! Everything is fine and really thanks again for your effort @umbertix!!! Before we proceed with the considerations we started with @reedjosh, I would really love to merge this one.

I just need to ask you to please make some slight modifications:

  • Squash all commits into one
  • The squashed commit has feat(charts): Add Jellifyn as commit message
  • In the helm-charts/k8s-mediaserver/Chart.yaml file can you please bump both versions to 0.10.0?

After this, I think we are good to go :)

Thank you @umbertix

@umbertix umbertix force-pushed the add-jellyfin branch 2 times, most recently from 30971c3 to a0d93cd Compare January 15, 2024 17:49
@kubealex
Copy link
Owner

@umbertix thank you SO MUCH!
All is working smooth, I just added a not about the Jackett probe that must be tcpSocket due to how probes work and k8s limits on redirects (Jackett needs cookies to avoid redirects).

I made the modification and ran the integration tests that are now super green :D

Can I just ask you two last things?

  • Can you add the README entries for Jellyfin vars?
  • Can you resquash everything into the feat(charts): Add Jellyfin commit?

I will just merge it afterwards, thank you!!

InputObject2
InputObject2 reviewed Jan 18, 2024
View reviewed changes
Copy link
Contributor

@InputObject2 InputObject2 left a comment

Choose a reason for hiding this comment

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

Just some nitpicks to consider changing.

Looks pretty good overall, great work!

helm-charts/k8s-mediaserver/templates/jackett-resources.yml Outdated
@@ -71,7 +71,7 @@ spec:
name: jackett-config
image: "{{ .Values.jackett.container.image }}:{{ .Values.jackett.container.tag | default .Values.general.image_tag }}"
imagePullPolicy: Always
readinessProbe:
readiness probe:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want to set this up with a space in the key. It would be different than all the other ones and also this isn't typically what people do since camel case is mostly the norm as far as I can tell.

helm-charts/k8s-mediaserver/values.yaml
@@ -279,3 +280,38 @@ plex:
# accessModes: ReadWriteOnce
# storage: 5Gi
# selector: {}

jellyfin:
enabled: true
Copy link
Contributor

Choose a reason for hiding this comment

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

We're enabling both plex and jellyfin by default? Seems like most users will use one or the other. I'd set it as enabled: false to give the option only.

@umbertix umbertix closed this Jan 28, 2024
@umbertix
Copy link
Contributor Author

Unfortunelty I fucked up my local git and I think I deleted the branch fully when messing with the squash. I most likely lost the work there so, don't expect this to come up any time soon.
Sorry :/

@kubealex
Copy link
Owner

if you still have your local repo with the history you can still try to recover it using:

git checkout -b add-jellyfin-recovery 04f2789

04f2789 is the last commit you made (the typo thingy) and even if its history is unrelated, you can still recover everything! @umbertix

@kubealex
Copy link
Owner

Similar situation depicted here

@reedjosh
Copy link

Lol @umbertix I was pointing to your branch with ArgoCD.

I woke up this morning to put a show on for the kiddos, but there was no longer a Jellyfin deployment in my k8s cluster. 🙃

@reedjosh reedjosh mentioned this pull request Jan 28, 2024
Draft
@umbertix
Copy link
Contributor Author

Thanks for the tips, I got some time today to fix this mess (I think). And I've opened a new PR #98 , I hope this one is the final one
Feel free to re-review it and test it to validate I haven't screwed any of the settings

Thx and sorry @reedjosh I hope ur kids weren't too mad at me ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@InputObject2 InputObject2 InputObject2 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] Jellyfin
4 participants
@umbertix @kubealex @reedjosh @InputObject2