fix: Enhance PR plugin script to handle file conflicts #2533
Conversation
…ckup restoration. Update build workflow to include additional directories and streamline tarball creation.
WalkthroughUpdates add pre-install conflict detection across webgui-pr-* plugins, broadened workflow packaging to include etc/sbin/share, and change plugin removal/restoration to non-destructive copy operations with missing-backup warnings and wider file-existence checks. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as PR Plugin Installer
participant Scanner as Manifest Scanner
participant OtherPlugins as Installed Plugins Manifests
participant Filesystem as System Files / Backups
Installer->>Scanner: Read new plugin file list
Installer->>OtherPlugins: Read each webgui-pr-*/installed_files.txt
Scanner-->>Installer: Build file→plugin mapping
Installer->>Scanner: Compare new files to mapping
alt Conflicts found
Scanner-->>Installer: Return conflict entries
Installer->>Filesystem: Print failure banner & guidance
Installer->>Installer: Clean temp files and exit (error)
else No conflicts
Installer->>Filesystem: Copy backups (cp -fp) / create NEW backups
Installer->>Filesystem: Install plugin files
Installer->>Scanner: Write installed_files.txt manifest
Installer-->>Installer: Complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/generate-pr-plugin.sh (1)
263-273: Inconsistent symlink handling in backup creation.Lines 89 and 402 use
-e "$system_file" || -L "$system_file"to handle symlinks, but line 266 uses only-f "$SYSTEM_FILE". If the system file is a symlink, it won't be backed up and will be marked as NEW, causing it to be deleted on removal rather than restored.Proposed fix
if [ -f "$BACKUP_FILE" ]; then echo " → Using existing backup: $BACKUP_FILE" echo "$SYSTEM_FILE|$BACKUP_FILE" >> "$MANIFEST" - elif [ -f "$SYSTEM_FILE" ]; then + elif [ -e "$SYSTEM_FILE" ] || [ -L "$SYSTEM_FILE" ]; then echo " → Creating backup of original: $SYSTEM_FILE" - cp -p "$SYSTEM_FILE" "$BACKUP_FILE" + cp -pP "$SYSTEM_FILE" "$BACKUP_FILE" echo "$SYSTEM_FILE|$BACKUP_FILE" >> "$MANIFEST" elseNote:
-pPpreserves symlinks as symlinks rather than following them.
🤖 Fix all issues with AI agents
In @.github/workflows/pr-plugin-build.yml:
- Around line 127-134: The tar step can fail if TAR_SOURCES is empty; update the
logic around the TAR_SOURCES array and the tar invocation (the TAR_SOURCES
variable and the tar -czf ../${{ steps.version.outputs.local_txz }}
"${TAR_SOURCES[@]}" line) to guard against an empty array: check the length of
TAR_SOURCES (e.g., ${`#TAR_SOURCES`[@]}) before calling tar and either create a
harmless placeholder (temporary empty directory/file) or skip/mark artifact
creation when no sources exist so tar is never invoked with an empty list.
| TAR_SOURCES=() | ||
| if [ -d etc ]; then | ||
| TAR_SOURCES+=("etc") | ||
| fi | ||
| if [ -d usr ]; then | ||
| TAR_SOURCES+=("usr") | ||
| fi | ||
| tar -czf ../${{ steps.version.outputs.local_txz }} "${TAR_SOURCES[@]}" |
There was a problem hiding this comment.
Handle empty TAR_SOURCES to prevent tarball creation failure.
If neither etc nor usr directories exist in the build folder (edge case), tar will fail with "Cowardly refusing to create an empty archive." While the current file mapping logic should always create usr for valid changes, consider adding a guard.
Proposed fix
TAR_SOURCES=()
if [ -d etc ]; then
TAR_SOURCES+=("etc")
fi
if [ -d usr ]; then
TAR_SOURCES+=("usr")
fi
+ if [ ${`#TAR_SOURCES`[@]} -eq 0 ]; then
+ echo "Error: No files to package"
+ exit 1
+ fi
tar -czf ../${{ steps.version.outputs.local_txz }} "${TAR_SOURCES[@]}"🤖 Prompt for AI Agents
In @.github/workflows/pr-plugin-build.yml around lines 127 - 134, The tar step
can fail if TAR_SOURCES is empty; update the logic around the TAR_SOURCES array
and the tar invocation (the TAR_SOURCES variable and the tar -czf ../${{
steps.version.outputs.local_txz }} "${TAR_SOURCES[@]}" line) to guard against an
empty array: check the length of TAR_SOURCES (e.g., ${`#TAR_SOURCES`[@]}) before
calling tar and either create a harmless placeholder (temporary empty
directory/file) or skip/mark artifact creation when no sources exist so tar is
never invoked with an empty list.
…arball creation process
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/pr-plugin-build.yml:
- Around line 121-130: The script can produce an empty file_list.txt which would
make tar create an unintended empty archive; before running the tar command (the
lines that cd build and call tar -czf ../${{ steps.version.outputs.local_txz }}
-T ../file_list.txt) add a guard that checks file_list.txt is non-empty (e.g.
test -s ../file_list.txt) and if it's empty print a clear message and exit with
non-zero status (or skip artifact creation) so the pipeline fails loudly instead
of producing an empty tar; update the surrounding echo/logging to reflect the
decision.
🧹 Nitpick comments (1)
.github/workflows/pr-plugin-build.yml (1)
41-43: Update log message to reflect all monitored directories.The message "No emhttp files changed" is misleading since the workflow now monitors
emhttp,etc,sbin, andsharedirectories (line 34).Proposed fix
if [ ! -s "changed_files.txt" ]; then - echo "No emhttp files changed" + echo "No relevant files changed in emhttp/etc/sbin/share" echo "has_changes=false" >> "$GITHUB_OUTPUT"
| # Generate file list for plugin/tar (relative paths) | ||
| find build -type f | sed 's|^build/||' > file_list.txt | ||
| echo "File list for plugin:" | ||
| cat file_list.txt | ||
| # Create tarball - consistent filename for updates | ||
| cd build | ||
| echo "Creating tarball with contents:" | ||
| find usr/ -type f | ||
| tar -czf ../${{ steps.version.outputs.local_txz }} usr/ | ||
| cat ../file_list.txt | ||
| tar -czf ../${{ steps.version.outputs.local_txz }} -T ../file_list.txt |
There was a problem hiding this comment.
Guard against empty file list before tar creation.
If all changed files fall into the "Skipping unsupported path" case (line 111-112), file_list.txt will be empty. While tar -T with an empty file may create an empty archive rather than failing, this likely isn't the desired behavior.
Proposed fix
# Generate file list for plugin/tar (relative paths)
find build -type f | sed 's|^build/||' > file_list.txt
+ if [ ! -s file_list.txt ]; then
+ echo "Error: No files to package"
+ exit 1
+ fi
echo "File list for plugin:"
cat file_list.txt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Generate file list for plugin/tar (relative paths) | |
| find build -type f | sed 's|^build/||' > file_list.txt | |
| echo "File list for plugin:" | |
| cat file_list.txt | |
| # Create tarball - consistent filename for updates | |
| cd build | |
| echo "Creating tarball with contents:" | |
| find usr/ -type f | |
| tar -czf ../${{ steps.version.outputs.local_txz }} usr/ | |
| cat ../file_list.txt | |
| tar -czf ../${{ steps.version.outputs.local_txz }} -T ../file_list.txt | |
| # Generate file list for plugin/tar (relative paths) | |
| find build -type f | sed 's|^build/||' > file_list.txt | |
| if [ ! -s file_list.txt ]; then | |
| echo "Error: No files to package" | |
| exit 1 | |
| fi | |
| echo "File list for plugin:" | |
| cat file_list.txt | |
| # Create tarball - consistent filename for updates | |
| cd build | |
| echo "Creating tarball with contents:" | |
| cat ../file_list.txt | |
| tar -czf ../${{ steps.version.outputs.local_txz }} -T ../file_list.txt |
🤖 Prompt for AI Agents
In @.github/workflows/pr-plugin-build.yml around lines 121 - 130, The script can
produce an empty file_list.txt which would make tar create an unintended empty
archive; before running the tar command (the lines that cd build and call tar
-czf ../${{ steps.version.outputs.local_txz }} -T ../file_list.txt) add a guard
that checks file_list.txt is non-empty (e.g. test -s ../file_list.txt) and if
it's empty print a clear message and exit with non-zero status (or skip artifact
creation) so the pipeline fails loudly instead of producing an empty tar; update
the surrounding echo/logging to reflect the decision.
SimonFair
left a comment
SimonFair
left a comment
There was a problem hiding this comment.
LTGM, have install and removed a few times working as expected.
Update build workflow to include additional directories and streamline tarball creation.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.