-
-
Notifications
You must be signed in to change notification settings - Fork 55
Update IMMICH_BASE to accept optional storage location #730
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?
Conversation
Make IMMICH_BASE variable assignment optional with an argument.
WalkthroughThe pull request modifies the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/modules/software/module_immich.sh (2)
140-142: Purge may target wrong directory if custom path isn't re-provided.If a user installed with a custom
IMMICH_BASE(e.g.,/mnt/storage/immich) but runspurgewithout the second argument, the command will attempt to delete the default path${SOFTWARE_FOLDER}/immichinstead—leaving the actual data behind or failing silently.Suggested approach: persist and retrieve IMMICH_BASE
"${commands[0]}") shift + + # Persist the configured IMMICH_BASE for future operations + echo "IMMICH_BASE=\"${IMMICH_BASE}\"" > "${SOFTWARE_FOLDER}/.immich_config" if ! pkg_installed docker-ce; thenThen at the top, before the case statement:
- IMMICH_BASE="${2:-"${SOFTWARE_FOLDER}/immich"}" # optional argument = immich storage location + # Load persisted config if exists, otherwise use default or provided argument + if [[ -f "${SOFTWARE_FOLDER}/.immich_config" ]] && [[ -z "$2" ]]; then + source "${SOFTWARE_FOLDER}/.immich_config" + else + IMMICH_BASE="${2:-"${SOFTWARE_FOLDER}/immich"}" + fi
151-159: Help text should document the optional storage location argument.The help output doesn't mention that
installaccepts an optional second argument for the storage location. Users won't discover this feature without reading the source.Proposed fix
"${commands[4]}") echo -e "\nUsage: ${module_options["module_immich,feature"]} <command>" echo -e "Commands: ${module_options["module_immich,example"]}" - echo -e "\tinstall\t- Install $title." + echo -e "\tinstall [path]\t- Install $title. Optionally specify storage location." echo -e "\tremove\t- Remove $title." echo -e "\tpurge\t- Purge $title data folder." echo -e "\tstatus\t- Installation status $title." echo -e "\thelp\t- Show this help message." echo ;;
🧹 Nitpick comments (1)
tools/modules/software/module_immich.sh (1)
36-36: Parameter expansion is correct, but the custom path isn't persisted.The bash syntax
${2:-"${SOFTWARE_FOLDER}/immich"}correctly defaults when$2is unset. However, the customIMMICH_BASEvalue is not persisted to a config file, so subsequent operations (especiallypurge) require the user to remember and re-provide the same path.Consider storing the configured path (e.g., in
"${SOFTWARE_FOLDER}/immich.conf"or similar) during install and reading it back forremove/purge/statusoperations to ensure consistency.
|
I think this would be useful to setup globally - not just for Immich, but for all. What do you think? |
You mean, move |
No. Software folder variable is common to all software titles and thus its overriding should be global. |
|
@igorpecovnik, I am not sure if I understood you.
|
Make IMMICH_BASE variable assignment optional with an argument.
Description
Usecase: Since immich can take huge storage, I want to keep it on a separate hard drive while I boot from a small SSD.
With this option, I can setup immich to store data in a custom location.
Issue reference:
fixes #729
Related documentation:
Add optional 2nd argument to
module_immich, which sets the base storage location of immich.Implementation Details
Provide a detailed description of the implementation. Include the following:
Documentation Summary
Metadata Included:
Did you include the metadata (associative arrays) in the code? Ensure that metadata for modules, jobs, and runtime has been updated appropriately.
Document Generated:
Did you generate the updated documentation using
armbian-configng --doc? Confirm if the command was run to updateREADME.mdand provide any relevant details.Testing Procedure
Describe the tests you ran to verify your changes. Provide relevant details about your test configuration.
Checklist