-
-
Notifications
You must be signed in to change notification settings - Fork 253
Multiple Container Fixes #2063
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
base: main
Are you sure you want to change the base?
Multiple Container Fixes #2063
Conversation
Fixes being able to install plugins Resolves pelican-dev#2024 Resolves pelican-dev#2025 Fixes symlink for plugins Adds symlink for server icons Resolves pelican-dev#2054 Fixes loading environment variables Resolves pelican-dev#2056 Integrates pelican-dev#2051 pelican-dev#2034 pelican-dev#2045 into a single PR.
📝 WalkthroughWalkthroughAdds runtime packages (zip, unzip, 7zip, bzip2-dev, yarn), adjusts symlinks for plugins and icons to persist under /pelican-data, and fixes .env loading in the entrypoint to split on newlines only; also creates /pelican-data/storage/icons at container startup. Changes
(Attention: verify plugin install flows that invoke composer/yarn at runtime and ensure permissions/ownership for newly created storage/icons and plugin paths.) 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docker/entrypoint.sh (1)
6-6: Consider a more robust environment loading approach.The
xargs -d '\n'fix handles spaces in values, but theexport $(...)pattern remains fragile with special characters (quotes, backticks, etc.). Consider using:- export $(grep -v '^#' .env | xargs -d '\n') # Fix is from https://github.com/pelican-dev/panel/pull/2045 + set -a + . .env + set +aThis approach sources the file directly with automatic exporting enabled, avoiding word-splitting issues entirely.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
DockerfileDockerfile.devdocker/entrypoint.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: QuintenQVD0
Repo: pelican-dev/panel PR: 2034
File: Dockerfile.base:12-12
Timestamp: 2025-12-24T09:21:58.780Z
Learning: Git is required at runtime in the Docker image for the plugin system's dependency installation, even though standard Packagist packages don't require it. This is needed in Dockerfile.base and other Dockerfiles.
📚 Learning: 2025-12-24T09:22:18.560Z
Learnt from: QuintenQVD0
Repo: pelican-dev/panel PR: 2034
File: Dockerfile.base:8-8
Timestamp: 2025-12-24T09:22:18.560Z
Learning: In the pelican-dev/panel project, the bz2 PHP extension is required in Dockerfile.base because a plugin in the plugin system depends on it for bzip2 compression functionality.
Applied to files:
DockerfileDockerfile.dev
📚 Learning: 2025-12-24T09:21:52.260Z
Learnt from: QuintenQVD0
Repo: pelican-dev/panel PR: 2034
File: Dockerfile.base:12-12
Timestamp: 2025-12-24T09:21:52.260Z
Learning: Ensure that Dockerfiles install git at runtime where required for plugin system dependency installation. Update Dockerfile.base and any other Dockerfiles that build runtime images to include git in the runtime dependencies (not just for build-time), so plugin installation steps function correctly in the container.
Applied to files:
Dockerfile.dev
🪛 Shellcheck (0.11.0)
docker/entrypoint.sh
[warning] 6-6: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (4)
docker/entrypoint.sh (1)
28-28: LGTM!The addition of
/pelican-data/storage/iconscorrectly ensures the directory exists for the new icons symlink, supporting the objective to persist icons across container restarts.Dockerfile.dev (2)
70-74: Verify git is installed for plugin system.The added packages (zip, unzip, 7zip, bzip2-dev, yarn) correctly support plugin installation per the PR objectives. However, based on learnings, git is required at runtime for the plugin system's dependency installation. Ensure git is included in the runtime packages.
🔎 Proposed fix to add git
RUN apk add --no-cache \ # packages for running the panel caddy ca-certificates supervisor supercronic fcgi coreutils \ # required for installing plugins. Pulled from https://github.com/pelican-dev/panel/pull/2034 - zip unzip 7zip bzip2-dev yarn + zip unzip 7zip bzip2-dev yarn gitBased on learnings, git is required at runtime for plugin system dependency installation.
93-93: LGTM!The icons symlink correctly ensures icon persistence across container restarts, directly addressing issue #2054.
Dockerfile (1)
88-88: LGTM!The icons symlink correctly ensures persistence across container restarts, addressing issue #2054.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other then the ln situation LGTM
Also it doesn't actually solves #2056 im still able to reproduce inside & outside of docker.
Co-authored-by: MartinOscar <40749467+rmartinoscar@users.noreply.github.com>
Co-authored-by: MartinOscar <40749467+rmartinoscar@users.noreply.github.com>
|
Out of interest, is there details on how the plugin system handles their composer packages? I couldn't find any code in the repo that suggested they are installed in the If they aren't installed into the I'm also not a heavy PHP developer, and so haven't worked with a plugin system that gives as much "power" as this one does - but I'm also wondering if plugins will have dependency conflicts if two plugins install different versions of the same plugin. |
Plugins currently install dependancies in the main app
If there's a conflict with existing plugin the new one will simplify fail to install until its resolved. @Boy132 correct me if im wrong. Good catch ! |
If multiple plugins have the same dependency but different versions, the version of the plugin that is loaded last will be used. panel/app/Services/Helpers/PluginService.php Lines 167 to 215 in bd012f5
|
|
when i'm trying this entrypoint.sh it shows up this error in the logs, but after the error it shows all .env vars as VARNAME='value' |
Fixes being able to install plugins
Resolves #2024
Resolves #2025
Fixes symlink for plugins
Resolves #2088
Adds symlink for server icons
Resolves #2054
Fixes loading environment variables
Resolves #2037
Integrates #2051 #2034 #2045 into a single PR.