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

First draft for Lyrion Music Server (LMS, Logitech Music Server) app #1243

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

hartzell
Copy link

@hartzell hartzell commented Dec 26, 2024

First draft for an app for the Lyrion Music Server (used to be Logitech Music Server).

Bits stolen from flaresolverr (extant app) and Parsonlabs Music (MR), chosen at random.

With the values in the basic-values.yaml test inputs file, this seems to work:

 .github/scripts/ci.py --app lms --train community --test-file basic-values.yaml

If I hack those answers up a bit and mount a real tree full of music into the volume then I can even index music.

I've marked this ready for review, but ... I need to get some feedback from the ixSystems folks.

TODO:

  • screenshots

FEEDBACK requests:

  • Other apps use perm containers to do things. Any docs about what/how/why/when?

@hartzell hartzell force-pushed the add-lyrion-music-server branch from a93423f to 34b0045 Compare December 26, 2024 21:21
@hartzell hartzell changed the title First draft for LMS app First draft for Lyrion Music Server (LMS, Logitech Music Server) app Dec 26, 2024
@hartzell hartzell force-pushed the add-lyrion-music-server branch from 34b0045 to ccaac32 Compare December 26, 2024 21:23
@hartzell hartzell marked this pull request as ready for review December 26, 2024 22:23
@kmoore134 kmoore134 requested a review from stavros-k January 2, 2025 13:54
@stavros-k
Copy link
Contributor

Hello, I'll take a look shortly.

Regarding the permissions, this is only needed when the container runs rootless.
In this case the container runs as root, and the sub processes of it run rootless. Still the pid 1 runs as root.
Eg if you see the upstream dockerfile https://github.com/LMS-Community/slimserver-platforms/blob/9c80a1dc89d8ec42dad5a7b28321c6db172d911b/Docker/start-container.sh#L5-L10

at startup it will create the user/group based on the PUID/PGID and then fix the permissions itself.

@hartzell
Copy link
Author

hartzell commented Jan 2, 2025

Thank you!

Copy link
Contributor

@stavros-k stavros-k 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 contribution! Left few comments.!
Overall looks fine, just small nits here and there.

Once those are fixed, I'll take a closer look as well!
Thansk!

ix-dev/community/lms/app.yaml Outdated Show resolved Hide resolved
ix-dev/community/lms/app.yaml Outdated Show resolved Hide resolved
ix-dev/community/lms/app.yaml Outdated Show resolved Hide resolved
ix-dev/community/lms/ix_values.yaml Outdated Show resolved Hide resolved
ix-dev/community/lms/questions.yaml Outdated Show resolved Hide resolved
Comment on lines 30 to 38
- variable: web_port
label: WebUI port
description: The port the web UI (default 9000).
schema:
type: int
default: 9000
required: true
$ref:
- definitions/port
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- variable: web_port
label: WebUI port
description: The port the web UI (default 9000).
schema:
type: int
default: 9000
required: true
$ref:
- definitions/port
- variable: web_port
label: WebUI Port
schema:
type: dict
attrs:
- variable: bind_mode
label: Port Bind Mode
description: |
The port bind mode.</br>
- Publish: The port will be published on the host for external access.</br>
- Expose: The port will be exposed for inter-container communication.</br>
- None: The port will not be exposed or published.</br>
Note: If the Dockerfile defines an EXPOSE directive,
the port will still be exposed for inter-container communication regardless of this setting.
schema:
type: string
default: "published"
enum:
- value: "published"
description: Publish port on the host for external access
- value: "exposed"
description: Expose port for inter-container communication
- value: ""
description: None
- variable: port_number
label: Port Number
schema:
type: int
show_if: [["bind_mode", "!=", ""]]
default: 31101
required: true
$ref:
- definitions/port

This is the new format for port

Copy link
Author

Choose a reason for hiding this comment

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

Done, but used 9000 as default, not the 31101 in your change. Am I mis-understanding?

ix-dev/community/lms/questions.yaml Outdated Show resolved Hide resolved
ix-dev/community/lms/templates/docker-compose.yaml Outdated Show resolved Hide resolved
ix-dev/community/lms/app.yaml Outdated Show resolved Hide resolved
ix-dev/community/lms/app.yaml Outdated Show resolved Hide resolved
@hartzell hartzell force-pushed the add-lyrion-music-server branch from 0d08619 to dee9fd2 Compare January 5, 2025 01:15
George Hartzell and others added 5 commits January 4, 2025 17:17
First draft for an app for the Lyrion Music Server (used to be
Logitech Music Server).

Bits stolen from flaresolverr (extant app) and Parsonlabs Music (MR),
chosen at random.

With the values in the `basic-values.yaml` test inputs file, this
seems to work:

    .github/scripts/ci.py --app lms --train community --test-file basic-values.yaml

If I had those answers up a bit and mount a real tree full of music
into the volume then I can even index music.

I need to get some feedback from the ixSystems folks.
- Add support for specifying web (9000) and CLI (9090) ports, leave
  IANA assigned slimproto ports hardwired.

- Improve storage handling, explicitly set up 3 storage bits for
  config, music, and playlist storage.
- name field in app.yaml
- rename ix-dev/community dir
- gid/uid (and names) should be root
- don't need run_as constants, they're configurable
- don't set PUID, GUID
- add run_as to questions and test answers
- fix health check bit in compose template
- "/" is the default, per PR review
- fix name in basic_values
- use new format definitions for web and cli ports
- UI only handles 1 category, so just give 1
- add capability info to app.yaml
@hartzell hartzell force-pushed the add-lyrion-music-server branch from dee9fd2 to 0d0c25f Compare January 5, 2025 01:18
@hartzell
Copy link
Author

hartzell commented Jan 5, 2025

@stavros-k -- I think I've addressed your feedback (assuming I did the right thing with the web port bit I left unresolved above). I can run the server using the generated docker-compose.yaml and it seems to work (haven't fed it any real streams or music yet though).

Thanks for helping me out!

@hartzell
Copy link
Author

hartzell commented Jan 5, 2025

url: https://www.truenas.com/
name: lyrion-music-server
run_as_context:
- description: Main pid runs as root
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- description: Main pid runs as root
- description: Lyrion Music Server runs as root user.

Copy link
Contributor

@stavros-k stavros-k left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Will check the port thingies in a bit and update the url for the images


consts:
lms_container_name: lms

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +24 to +26
config: /mnt/storage/lms/config
music: /mnt/storage/lms/music
playlist: /mnt/storage/lms/playlist
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
config: /mnt/storage/lms/config
music: /mnt/storage/lms/music
playlist: /mnt/storage/lms/playlist
config: /opt/tests/mnt/storage/lms/config
music: /opt/tests/mnt/storage/lms/music
playlist: /opt/tests/mnt/storage/lms/playlist

Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly to keep it consistent with the rest.
I dont want someone to accidentaly run the tests and files in their /mnt/storage are getting written/overwritten

required: true
$ref:
- definitions/timezone

Copy link
Contributor

Choose a reason for hiding this comment

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

missing the variable: lms section with the addtitional envs section

groups:
- name: LMS Configuration
description: Configure Lyrion Music Server
- name: Network Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing the user group here

@stavros-k
Copy link
Contributor

For screenshots, how about

* https://lyrion.org/assets/screenshot.png

And for an icon:

* https://raw.githubusercontent.com/LMS-Community/slimserver-platforms/public/9.0/win32/res/SqueezeCenter.ico

Does the app have a webgui? or is it a server only?
If its a server only, I dont think we should add a screenshot of the client here.

description: Lyrion Music Server - controls a wide range of Squeezebox audio players
home: https://lyrion.org/
host_mounts: []
icon: https://avatars.githubusercontent.com/u/138057124?s=200&v=4
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
icon: https://avatars.githubusercontent.com/u/138057124?s=200&v=4
icon: https://media.sys.truenas.net/apps/lyrion-music-server/icons/icon.png

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.

2 participants