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

Add Emby #221

Merged
merged 14 commits into from
Mar 8, 2021
Merged

Add Emby #221

merged 14 commits into from
Mar 8, 2021

Conversation

djs52
Copy link
Contributor

@djs52 djs52 commented Mar 5, 2021

Description

Add Emby Media Server, based on the Jellyfin chart, using the linuxserver.io Docker image. Fixes #64

Type of change

  • Feature/App addition
  • Bugfix
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor of current code

How Has This Been Tested?

Installed and running perfectly on TrueNAS-SCALE-21.03-MASTER-20210304-232915

Notes:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests to this description that prove my fix is effective or that my feature works

@djs52 djs52 requested a review from Ornias1993 as a code owner March 5, 2021 10:40
Copy link
Member

@Ornias1993 Ornias1993 left a comment

Choose a reason for hiding this comment

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

Looks quite good, some small nitpicks. 💯
However, I can't accept this to master, as master does not accept new apps due to the refactor going on.
Accepting this would mean I would directly need to rewrite your questions.yaml, values.yaml and test_values.yaml and I rather focus on getting the refactors done ;-)

Please submit/pr this to "staging" instead. (as requested on the IX forums)

Besides that big thing and the nitpicks, it's quite good.
If it wheren't for the refactor it would be merged already!

Also:
1 more nit:

  • if you want to be the maintainer, try adding yourself to .github/CODEOWNERS

Upside:
The documentation and testing in the staging branch is much improved for the things that are refactored :)

charts/emby/1.6.3/Chart.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,55 @@
# Introduction
Copy link
Member

Choose a reason for hiding this comment

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

Don't worry about this file, it gets automatically replaced/removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I drop it from the PR?

Copy link
Member

Choose a reason for hiding this comment

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

No it needs to be there, don't worry about it was all I was saying.

@@ -0,0 +1,2 @@
Emby Server is a home media server
Copy link
Member

Choose a reason for hiding this comment

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

This file also gets automatically removed/replaced

Comment on lines 12 to 14
- jellyfin
- plex
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 it's fair to add keywords for totally different solutions...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh :) I literally copied this list from the jellyfin chart! I was a bit surprised, but figured it was fair game.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah needs to go there too... thanks for the headsup! :)

charts/emby/item.yaml Outdated Show resolved Hide resolved
@djs52 djs52 changed the base branch from master to staging March 5, 2021 11:54
@djs52
Copy link
Contributor Author

djs52 commented Mar 5, 2021

Looks quite good, some small nitpicks.
However, I can't accept this to master, as master does not accept new apps due to the refactor going on.
Accepting this would mean I would directly need to rewrite your questions.yaml, values.yaml and test_values.yaml and I rather focus on getting the refactors done ;-)

That's fair. Anything I need to do to prepare this for the post-refactor world?

Please submit/pr this to "staging" instead. (as requested on the IX forums)

Hah, I hadn't even noticed Truecharts had its own thread. Sorry about that, switched the PR over.

Besides that big thing and the nitpicks, it's quite good.

Thanks :)

@djs52 djs52 requested a review from Ornias1993 March 5, 2021 12:13
@Ornias1993
Copy link
Member

I'm going to consider this blocked till #219 gets merged into staging...
As thats the second part of the two-part big breaking-change refactor.

Comment on lines 12 to 14
- jellyfin
- plex
Copy link
Member

Choose a reason for hiding this comment

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

Yeah needs to go there too... thanks for the headsup! :)

Comment on lines 20 to 27
dependencies:
- name: common
repository: https://charts.truecharts.org/
version: 1.6.5
# condition:
# tags:
# import-values:
# alias:
Copy link
Member

Choose a reason for hiding this comment

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

for staging this needs to be 2.0.0
(and needs to be manually copied)

@@ -0,0 +1,55 @@
# Introduction
Copy link
Member

Choose a reason for hiding this comment

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

No it needs to be there, don't worry about it was all I was saying.


## TrueCharts Specific

- variable: appVolumeMounts
Copy link
Member

Choose a reason for hiding this comment

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

Make sure these follow the standardised documented format on the staging branch (.github/docs is the root folder for the docs for now, will be moved on staging to docs/)

schema:
type: string

- variable: service
Copy link
Member

Choose a reason for hiding this comment

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

Make sure these follow the standardised documented format on the staging branch (.github/docs is the root folder for the docs for now, will be moved on staging to docs/).

This one currently does not.

@Ornias1993
Copy link
Member

I should have #219 merged in a day or two, I'll make it my prime priority to merge this as soon as possible afterwards.

@Ornias1993 Ornias1993 removed the Blocked label Mar 5, 2021
@Ornias1993
Copy link
Member

Ingress refactor (#219) has been rushed and merged into staging.
You should now have the right documentation and tool you need :)

The previous faq url points to a 404 page.
Also added a link to the Github issue tracker.
@Ornias1993
Copy link
Member

Ornias1993 commented Mar 6, 2021

I managed to sprint #233 into it now too, all big changes have been merged and you should be fine to redo this for the staging branch @djs52 👍

@djs52
Copy link
Contributor Author

djs52 commented Mar 6, 2021

Awesome, thanks! I might not be able to get back to this until Monday, sorry.

@Ornias1993
Copy link
Member

@djs52 No problem at all, we all have other things to do too :)

@Ornias1993
Copy link
Member

Ornias1993 commented Mar 8, 2021

Try not to merge upstream/master into this one, you are really dirtying your PR...
Try rebasing...

Also: Don't submit PR's from your personal master branch, always keep master prestine ;-)

@djs52
Copy link
Contributor Author

djs52 commented Mar 8, 2021

Yeah, I've made a bit of a mess of things. Let me see if I can unpick this, or if I have to recreate the PR from scratch.

@Ornias1993
Copy link
Member

@djs52 No problem, just take your time :)

@djs52 djs52 requested a review from Ornias1993 March 8, 2021 15:14
Copy link
Member

@Ornias1993 Ornias1993 left a comment

Choose a reason for hiding this comment

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

Only two little nitpicks :)

@@ -0,0 +1,6 @@
dependencies:
Copy link
Member

Choose a reason for hiding this comment

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

remove the lockfile, it's not compatbile (and actually not needed either atm.)

charts/emby/2.0.0/ix_values.yaml Outdated Show resolved Hide resolved
charts/emby/2.0.0/test_values.yaml Outdated Show resolved Hide resolved
djs52 and others added 3 commits March 8, 2021 19:17
Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
@djs52
Copy link
Contributor Author

djs52 commented Mar 8, 2021

Done, thanks for your time :)

@djs52 djs52 requested a review from Ornias1993 March 8, 2021 19:21
@Ornias1993 Ornias1993 merged commit 467fd98 into truecharts:staging Mar 8, 2021
Ornias1993 pushed a commit that referenced this pull request Mar 12, 2021
* Fix FAQ link, add issue tracker link (#226)

The previous faq url points to a 404 page.
Also added a link to the Github issue tracker.

* Simple Emby chart based on the existing Jellyfin chart

* Set current UID and GID environment variables

* Reset the version to match the other charts

* Clean up incorrect version

* Fix versions and maintainer in Chart.yaml# Please enter the commit message for your changes. Lines starting

* Switch to the linuxserver.io Docker image -- it's more closely matched to other Truecharts

* Add end of file \n

* Add back upstream_version (commented out)

* Update CODEOWNERS

* Update Emby for Truecharts 2.0.0

* Update charts/emby/2.0.0/ix_values.yaml

Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>

* Update charts/emby/2.0.0/test_values.yaml

Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>

* Delete Chart.lock

Co-authored-by: Sebastien Dupont <dupont.sebastien@gmail.com>
Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
Ornias1993 pushed a commit that referenced this pull request Mar 17, 2021
* Fix FAQ link, add issue tracker link (#226)

The previous faq url points to a 404 page.
Also added a link to the Github issue tracker.

* Simple Emby chart based on the existing Jellyfin chart

* Set current UID and GID environment variables

* Reset the version to match the other charts

* Clean up incorrect version

* Fix versions and maintainer in Chart.yaml# Please enter the commit message for your changes. Lines starting

* Switch to the linuxserver.io Docker image -- it's more closely matched to other Truecharts

* Add end of file \n

* Add back upstream_version (commented out)

* Update CODEOWNERS

* Update Emby for Truecharts 2.0.0

* Update charts/emby/2.0.0/ix_values.yaml

Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>

* Update charts/emby/2.0.0/test_values.yaml

Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>

* Delete Chart.lock

Co-authored-by: Sebastien Dupont <dupont.sebastien@gmail.com>
Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
Ornias1993 pushed a commit that referenced this pull request Mar 31, 2021
* Fix FAQ link, add issue tracker link (#226)

The previous faq url points to a 404 page.
Also added a link to the Github issue tracker.

* Simple Emby chart based on the existing Jellyfin chart

* Set current UID and GID environment variables

* Reset the version to match the other charts

* Clean up incorrect version

* Fix versions and maintainer in Chart.yaml# Please enter the commit message for your changes. Lines starting

* Switch to the linuxserver.io Docker image -- it's more closely matched to other Truecharts

* Add end of file \n

* Add back upstream_version (commented out)

* Update CODEOWNERS

* Update Emby for Truecharts 2.0.0

* Update charts/emby/2.0.0/ix_values.yaml

Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>

* Update charts/emby/2.0.0/test_values.yaml

Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>

* Delete Chart.lock

Co-authored-by: Sebastien Dupont <dupont.sebastien@gmail.com>
Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
Ornias1993 pushed a commit that referenced this pull request Mar 31, 2021
* Fix FAQ link, add issue tracker link (#226)

The previous faq url points to a 404 page.
Also added a link to the Github issue tracker.

* Simple Emby chart based on the existing Jellyfin chart

* Set current UID and GID environment variables

* Reset the version to match the other charts

* Clean up incorrect version

* Fix versions and maintainer in Chart.yaml# Please enter the commit message for your changes. Lines starting

* Switch to the linuxserver.io Docker image -- it's more closely matched to other Truecharts

* Add end of file \n

* Add back upstream_version (commented out)

* Update CODEOWNERS

* Update Emby for Truecharts 2.0.0

* Update charts/emby/2.0.0/ix_values.yaml

Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>

* Update charts/emby/2.0.0/test_values.yaml

Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>

* Delete Chart.lock

Co-authored-by: Sebastien Dupont <dupont.sebastien@gmail.com>
Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
@Ornias1993
Copy link
Member

@all-contributors please add @djs52 for code

@allcontributors
Copy link
Contributor

@Ornias1993

I've put up a pull request to add @djs52! 🎉

ellienieuwdorp pushed a commit to ellienieuwdorp/charts that referenced this pull request Apr 10, 2021
* Fix FAQ link, add issue tracker link (truecharts#226)

The previous faq url points to a 404 page.
Also added a link to the Github issue tracker.

* Simple Emby chart based on the existing Jellyfin chart

* Set current UID and GID environment variables

* Reset the version to match the other charts

* Clean up incorrect version

* Fix versions and maintainer in Chart.yaml# Please enter the commit message for your changes. Lines starting

* Switch to the linuxserver.io Docker image -- it's more closely matched to other Truecharts

* Add end of file \n

* Add back upstream_version (commented out)

* Update CODEOWNERS

* Update Emby for Truecharts 2.0.0

* Update charts/emby/2.0.0/ix_values.yaml

Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>

* Update charts/emby/2.0.0/test_values.yaml

Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>

* Delete Chart.lock

Co-authored-by: Sebastien Dupont <dupont.sebastien@gmail.com>
Co-authored-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
@truecharts-admin
Copy link
Collaborator

This PR is locked to prevent necro-posting on closed PRs. Please create a issue or contact staff on discord if you want to further discuss this

@truecharts truecharts locked as resolved and limited conversation to collaborators Jul 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Emby App
4 participants