66 add automatic plugin type detection in cli plugin installation #67
Conversation
WalkthroughThe pull request introduces several modifications across three files. The workflow for checking Moodle versions has been enhanced to filter out pre-release tags, ensuring only stable versions are considered. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
erseco
deleted the
66-add-automatic-plugin-type-detection-in-cli-plugin-installation
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
.github/workflows/check-moodle-version.yml (1)
27-28: Approve changes with a minor suggestion for improvementThe modification to filter out pre-release tags (containing '-rc', '-beta', or '-alpha') when determining the current version is a good improvement. It ensures that only stable releases are considered, which will lead to more accurate version comparisons.
Consider expanding the regex pattern to catch potential additional pre-release formats. For example:
- CURRENT_VERSION=$(git tag | grep -vE '(-rc|-beta|-alpha)' | sort -V | tail -n 1) + CURRENT_VERSION=$(git tag | grep -vE '(-rc|-beta|-alpha|-pre|-dev)' | sort -V | tail -n 1)This would also exclude tags with '-pre' or '-dev', which are sometimes used for pre-release versions.
rootfs/var/www/html/admin/cli/install_plugin.php (1)
Line range hint
226-260: Refactor Suggestion: Implement automatic plugin type detection fromversion.php.Currently, the script requires users to specify the plugin type using the
--typeoption, defaulting to'mod'. However, the plugin type can be inferred from the$plugin->componentin theversion.phpfile of the plugin.Solution:
Modify the
detect_plugin_namefunction to return both the plugin name and type:function detect_plugin_name($dir) { $versionFile = $dir . '/version.php'; if (!file_exists($versionFile)) { return false; } $plugin = new stdClass(); include($versionFile); if (!isset($plugin->component)) { return false; } - return $plugin->component; + list($type, $name) = explode('_', $plugin->component, 2); + return [ + 'component' => $plugin->component, + 'type' => $type, + 'name' => $name, + ]; }Adjust the script to utilize the detected type:
- $pluginname = detect_plugin_name("$tempdir/$rootdir"); + $plugininfo = detect_plugin_name("$tempdir/$rootdir"); - $destination = detect_and_move_plugin($pluginname, "$tempdir/$rootdir", $options['type']); + $destination = detect_and_move_plugin($plugininfo['name'], "$tempdir/$rootdir", $plugininfo['type']);Remove the
--typeoption from the help text and options array since it's no longer needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/check-moodle-version.yml (1 hunks)
- .gitignore (1 hunks)
- rootfs/var/www/html/admin/cli/install_plugin.php (7 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🔇 Additional comments (2)
.github/workflows/check-moodle-version.yml (1)
Line range hint
1-62: Suggestions for overall workflow improvementsWhile the current changes improve the version checking process, here are some suggestions to enhance the overall workflow:
Error Handling: Consider adding error checks for critical operations, especially for the curl commands and git operations. This will make the workflow more robust and easier to debug.
Version Parsing: The current method assumes the tag name is the version. Consider adding a step to parse the version from the tag name, in case the tag format changes in the future.
Rate Limiting: Add handling for potential API rate limiting issues when making requests to the GitHub API.
Here's an example of how you could implement error handling for the curl command:
- name: Check Moodle Latest Version id: check_moodle run: | LATEST_MOODLE_VERSION=$(curl -s https://api.github.com/repos/moodle/moodle/tags | jq -r '.[0].name') if [ -z "$LATEST_MOODLE_VERSION" ]; then echo "Failed to fetch latest Moodle version" exit 1 fi echo "LATEST_MOODLE_VERSION=$LATEST_MOODLE_VERSION" >> $GITHUB_ENVTo ensure the workflow is correctly handling different tag formats, we can check the existing tags in the repository:
This will help verify if there are any tag formats that our current filtering might miss.
✅ Verification successful
Verified Tag Formats
The existing tags in the Moodle repository adhere to the expected versioning pattern, including pre-release tags such as
vX.Y.Z-betaandvX.Y.Z-rc1. The workflow effectively filters out these pre-release tags when determining the current version, ensuring accurate version comparisons.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check existing tag formats in the repository # Test: List all tags and their formats git ls-remote --tags https://github.com/moodle/moodle.git | awk '{print $2}' | sed 's/refs\/tags\///' | sort -VLength of output: 9866
🧰 Tools
🪛 actionlint
26-26: shellcheck reported issue in this script: SC2086:info:3:44: Double quote to prevent globbing and word splitting
(shellcheck)
rootfs/var/www/html/admin/cli/install_plugin.php (1)
51-54: Verify: Ensure that debug messages are properly displayed in CLI mode.The
set_debugging(DEBUG_DEVELOPER, true);function is called to enable debug mode. However, in CLI scripts, additional settings might be required to display debug messages effectively.To Verify:
Run the script with the
--debugoption and check if debug messages are displayed as expected.If debug messages are not shown, consider adding
define('CLI_DEFER_INIT', true);at the beginning of the script and callingkillme();after setting debugging to re-initialize output settings.
| --run Execute install. If this option is not set, the script will run in dry mode. | ||
| --force Force install even if plugin exists (useful for development). | ||
| --debug Enable debug mode to see detailed error messages. |
There was a problem hiding this comment.
Issue: The --force option is specified but not implemented.
The --force option is listed in the help text on line 17, but the script does not utilize $options['force'] anywhere in the code. As a result, this option has no effect on the plugin installation process.
Solution:
Implement logic to handle the --force option when checking for existing plugins. This would allow users to overwrite existing plugin directories if they specify the --force flag.
| if (!$options['url']) { | ||
| cli_writeln('You must specify a URL to download the plugin.'); |
There was a problem hiding this comment.
Issue: --plugin option appears unsupported but is still referenced.
The script now mandates the --url option and exits if it's not provided. However, the help text and examples still reference a --plugin option, which seems unsupported in the current code.
Solution:
- Option 1: Re-implement support for the
--pluginoption if it's intended to be used. - Option 2: Update the help text and examples to remove references to the
--pluginoption, ensuring consistency.
| $destination = detect_and_move_plugin($pluginname, "$tempdir/$rootdir", $options['type']); | ||
| if (!$destination) { | ||
| cli_error('Failed to move plugin to the correct directory.'); |
There was a problem hiding this comment.
Issue: Duplicate or conflicting calls to detect_and_move_plugin.
There are two calls to detect_and_move_plugin:
- Lines 107-109: Uses
$tempdir/$rootdiras the source path. - Lines 112-114: Uses
$pluginDiras the source path, but$pluginDiris not defined.
Solution:
- Confirm the correct source path for moving the plugin.
- Remove the duplicate or incorrect call.
- Define
$pluginDirif it's necessary, or correct it to use the appropriate variable.
| // Move the plugin to the correct directory | ||
| if (rename($sourcepath, $destination)) { | ||
| cli_writeln("Plugin moved to $destination."); | ||
| return $destination; | ||
| } else { | ||
| return false; |
There was a problem hiding this comment.
Issue: Potential overwriting of existing plugins without handling --force option.
The rename($sourcepath, $destination) function may overwrite an existing plugin directory without checking if it already exists or considering the --force option.
Solution:
Before moving the plugin, check if the destination directory exists and handle it accordingly:
if (file_exists($destination)) {
if ($options['force']) {
// Optionally, remove the existing directory.
// Be cautious with recursive deletions.
remove_dir($destination); // Implement this function or use an existing one.
} else {
cli_error("Plugin already exists at $destination. Use --force to overwrite.");
}
}Ensure that you safely handle directory deletions and inform the user appropriately.
| $destination = detect_and_move_plugin($pluginname, $pluginDir, $options['type']); | ||
| if (!$destination) { | ||
| cli_error('Failed to move plugin to the correct directory.'); |
There was a problem hiding this comment.
Issue: Undefined variable $pluginDir used in function call.
The variable $pluginDir is not defined before it's used in detect_and_move_plugin($pluginname, $pluginDir, $options['type']); on line 112. This will lead to an undefined variable error.
Solution:
Replace $pluginDir with the correct variable. If you intended to use $tempdir/$rootdir, ensure consistency:
- $destination = detect_and_move_plugin($pluginname, $pluginDir, $options['type']);
+ $destination = detect_and_move_plugin($pluginname, "$tempdir/$rootdir", $options['type']);📝 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.
| $destination = detect_and_move_plugin($pluginname, $pluginDir, $options['type']); | |
| if (!$destination) { | |
| cli_error('Failed to move plugin to the correct directory.'); | |
| $destination = detect_and_move_plugin($pluginname, "$tempdir/$rootdir", $options['type']); | |
| if (!$destination) { | |
| cli_error('Failed to move plugin to the correct directory.'); |
| // Ensure the directory is writable before moving the plugin | ||
| if (!is_writable($destination)) { | ||
| cli_error("Directory $destination is not writable. Check permissions."); | ||
| } |
There was a problem hiding this comment.
Issue: Incorrect check for directory writability on potentially non-existent path.
The check is_writable($destination) may fail if $destination does not exist yet. Since you intend to move the plugin to $destination, you should check if the parent directory is writable.
Solution:
Modify the check to verify the parent directory's writability:
- if (!is_writable($destination)) {
+ if (!is_writable(dirname($destination))) {This ensures that the script correctly checks the ability to create the destination directory.
📝 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.
| // Ensure the directory is writable before moving the plugin | |
| if (!is_writable($destination)) { | |
| cli_error("Directory $destination is not writable. Check permissions."); | |
| } | |
| // Ensure the directory is writable before moving the plugin | |
| if (!is_writable(dirname($destination))) { | |
| cli_error("Directory $destination is not writable. Check permissions."); | |
| } |
Summary by CodeRabbit
New Features
--type=<plugintype>and--debugfor improved flexibility and error handling.Chores
.gitignoreto exclude files starting with.aider.