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 ParsonLabs Music to Application Catalog #1074

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

WillKirkmanM
Copy link

@WillKirkmanM WillKirkmanM commented Nov 29, 2024

home-w
home-te
explore-page
album_showcase_wca
library-showcase
music-album-md
pfp-showcase
genres-showcase
music-video-home
plm-complete
genre-selection
music-video
home-plm
updated-lyrics
updated-artist
ArtistAlbumSearch
profile
search
lyrics-showcase
music-artist-showcase
music-in-your-library
music-playlist

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.

Hello, this does not seem to work at all.
I cannot even render the template.

Image is recommended to use a versioned tag and not latest.
Port is static so users cannot edit it. Also it would be nice if internal container port is also changed via env variable to match the external one.
So users can also use host network without chances of port conflicts.

certificate is not used anywhere

container name is different in the labels section than the one that is used to create the container.

values are statically set in the compose file instead of using the values file.

For readme I'd suggest having a small description like the rest of the apps,
and have a link to your github for the rest. (as the rest of the apps)

app.yaml file should be the same format as the rest, as it is modified from automated tools and will create big diffs on subsequent automated PRs

questions.yaml seems to have a user/group group defined at the top but is not used anywhere.
Is the container running as root or rootless?
Based on the compose it seems to run as rootless, but uses cannot edit that. as you reference it from the consts.

@stavros-k stavros-k marked this pull request as draft December 2, 2024 17:29
@WillKirkmanM
Copy link
Author

Thanks for the review, will fix.

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