Drupal101 fix install fail scripts early #132
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThe changes introduced in this pull request primarily focus on enhancing error handling, optimizing command execution, and refining the setup process for Drupal projects. The scripts have been updated to handle errors more strictly, initialize variables with default values, and optimize composer commands. Additionally, the Drupal setup script has been improved to support patch application, automatic configuration updates, and timed command execution. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (3)
- .gitpod/drupal/drupalpod-setup.sh (13 hunks)
- .gitpod/drupal/install-essential-packages.sh (2 hunks)
- .gitpod/utils/set-base-environment.sh (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitpod/utils/set-base-environment.sh
Additional comments (Suppressed): 13
.gitpod/drupal/install-essential-packages.sh (2)
5-5: The addition of
set -eu -o pipefailis a good practice for shell scripts. It ensures that the script will exit immediately if any command fails, treats unset variables as an error and causes a pipeline to fail if any of the commands fail. This can help catch errors early and prevent them from causing further issues down the line.15-15: The
-Wflag has been added to the composer require command. This flag updates dependencies within the specified constraints. Ensure that this change does not cause any unexpected behavior with the installed packages.- cd "${GITPOD_REPO_ROOT}" && time ddev . composer require --dev "drupal/core-dev":* "phpspec/prophecy-phpunit":^2 --no-install + cd "${GITPOD_REPO_ROOT}" && time ddev . composer require --dev "drupal/core-dev":* "phpspec/prophecy-phpunit":^2 -W --no-install.gitpod/drupal/drupalpod-setup.sh (11)
- 3-5: The condition for enabling debug mode has been changed. Previously, the script would enable debug mode if either
DEBUG_SCRIPTorGITPOD_HEADLESSwas set (not empty). Now, it enables debug mode if either of these variables is unset (empty). This change could lead to unexpected behavior if other parts of the system rely on the previous logic.- if [ -n "$DEBUG_SCRIPT" ] || [ -n "$GITPOD_HEADLESS" ]; then + if [ "$DEBUG_SCRIPT" = '' ] || [ "$GITPOD_HEADLESS" = '' ]; then
16-31: The initialization of variables has been improved by assigning an empty string to non-existent variables. This is a good practice as it prevents potential issues with unset variables later in the script.
34-37: The condition for setting default setup during the prebuild process has been changed. Previously, the script would set the default setup if
GITPOD_HEADLESSwas set (not empty). Now, it sets the default setup ifGITPOD_HEADLESSis unset (empty). This change could lead to unexpected behavior if other parts of the system rely on the previous logic.- if [ -n "$GITPOD_HEADLESS" ]; then + if [ "$GITPOD_HEADLESS" = '' ]; then
53-56: The comparison operator for checking the Drupal core version has been changed from
==to=. In Bash, both==and=are valid for string comparison in conditional expressions, so this change should not affect the functionality.64-65: The condition for skipping setup has been changed. Previously, the script would skip setup if it already ran once and if
DP_PROJECT_TYPEwas set (not empty). Now, it skips setup if it already ran once and ifDP_PROJECT_TYPEis unset (empty). This change could lead to unexpected behavior if other parts of the system rely on the previous logic.- if [ ! -f "${GITPOD_REPO_ROOT}"/.drupalpod_initiated ] && [ -n "$DP_PROJECT_TYPE" ]; then + if [ ! -f "${GITPOD_REPO_ROOT}"/.drupalpod_initiated ] && [ "$DP_PROJECT_TYPE" = '' ]; then
78-81: The comparison operator for checking the project type has been changed from
==to=. In Bash, both==and=are valid for string comparison in conditional expressions, so this change should not affect the functionality.119-122: The condition for checking out a specific branch has been changed. Previously, the script would checkout a specific branch if
DP_ISSUE_BRANCHwas set (not empty). Now, it checks out a specific branch ifDP_ISSUE_BRANCHis unset (empty). This change could lead to unexpected behavior if other parts of the system rely on the previous logic.- if [ -n "$DP_ISSUE_BRANCH" ]; then + if [ "$DP_ISSUE_BRANCH" = '' ]; then
- 128-130: The condition for checking out a module version has been changed. Previously, the script would checkout a module version if
DP_MODULE_VERSIONwas set (not empty) andDP_PROJECT_TYPEwas not equal to "project_core". Now, it checks out a module version ifDP_MODULE_VERSIONis unset (empty) andDP_PROJECT_TYPEis not equal to "project_core". This change could lead to unexpected behavior if other parts of the system rely on the previous logic.- elif [ -n "$DP_MODULE_VERSION" ] && [ "$DP_PROJECT_TYPE" != "project_core" ]; then + elif [ "$DP_MODULE_VERSION" = '' ] && [ "$DP_PROJECT_TYPE" != "project_core" ]; then
- 179-182: The condition for adding project source code as a symlink has been changed. Previously, the script would add the project source code as a symlink if
DP_PROJECT_NAMEwas set (not empty). Now, it adds the project source code as a symlink ifDP_PROJECT_NAMEis unset (empty). This change could lead to unexpected behavior if other parts of the system rely on the previous logic.- if [ -n "$DP_PROJECT_NAME" ]; then + if [ "$DP_PROJECT_NAME" = '' ]; then
190-193: The comparison operator for checking the project type has been changed from
==to=. In Bash, both==and=are valid for string comparison in conditional expressions, so this change should not affect the functionality.237-245: The condition for preparing special setup for Drupal projects with no composer.json file has been changed. Previously, the script would prepare the special setup if
DP_PROJECT_NAMEwas set (not empty). Now, it prepares the special setup ifDP_PROJECT_NAMEis unset (empty). This
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Files selected for processing (1)
- .gitpod/drupal/drupalpod-setup.sh (10 hunks)
Additional comments (Suppressed): 5
.gitpod/drupal/drupalpod-setup.sh (5)
2-3: The use of
set -eu -o pipefailis a good practice for stricter error handling in bash scripts. It ensures that the script will exit immediately if any command fails or if an unset variable is used.5-19: Initializing all variables with default values using
${VAR:=}syntax is a good practice to avoid potential issues with unset variables. This can help improve the robustness of the script.22-24: The use of
set -xwhen either$DEBUG_SCRIPTor$GITPOD_HEADLESSis set is a good practice for debugging purposes. It will print each command that is executed to the terminal, which can be helpful for troubleshooting.26-26: The
ddev startcommand is now being run directly instead of in the background. Ensure that this change does not impact the expected behavior of the script, as it may cause the script to wait until theddev startcommand has completed before proceeding.148-159: The handling of patch files seems to have been updated. Ensure that this new implementation works as expected and does not introduce any issues.
.gitpod/drupal/drupalpod-setup.sh
Outdated
| if [[ "$DP_CORE_VERSION" = 10* ]]; then | ||
| # admin_toolbar not compatible yet with Drupal 10 | ||
| # unset DP_EXTRA_ADMIN_TOOLBAR | ||
| if [ "$DP_PROJECT_TYPE" != "project_core" ]; then |
There was a problem hiding this comment.
The condition check for Drupal 10 has been updated from == to =. In bash scripting, both == and = are used for string comparison. However, == is more commonly used and is more readable, so consider reverting this change.
- if [[ "$DP_CORE_VERSION" = 10* ]]; then
+ if [[ "$DP_CORE_VERSION" == 10* ]]; then
.gitpod/drupal/drupalpod-setup.sh
Outdated
| if [ "$DP_PROJECT_TYPE" = "project_core" ]; then | ||
| # Find if requested core version is dev or stable | ||
| d="$DP_CORE_VERSION" | ||
| case $d in |
There was a problem hiding this comment.
The condition check for project type has been updated from == to =. As mentioned above, consider reverting this change for readability.
- if [ "$DP_PROJECT_TYPE" = "project_core" ]; then
+ if [ "$DP_PROJECT_TYPE" == "project_core" ]; then
.gitpod/drupal/drupalpod-setup.sh
Outdated
| if [ "$DP_PROJECT_TYPE" = "project_core" ]; then | ||
| # Add a special path when working on core contributions | ||
| # (Without it, /web/modules/contrib is not found by website) | ||
| cd "${GITPOD_REPO_ROOT}" && |
There was a problem hiding this comment.
Again, the condition check for project type has been updated from == to =. Consider reverting this change for readability.
- if [ "$DP_PROJECT_TYPE" = "project_core" ]; then
+ if [ "$DP_PROJECT_TYPE" == "project_core" ]; then
.gitpod/drupal/drupalpod-setup.sh
Outdated
| if [ "$DP_PROJECT_TYPE" = "project_module" ]; then | ||
| PROJECT_TYPE=modules | ||
| elif [ "$DP_PROJECT_TYPE" == "project_theme" ]; then | ||
| elif [ "$DP_PROJECT_TYPE" = "project_theme" ]; then | ||
| PROJECT_TYPE=themes | ||
| fi |
There was a problem hiding this comment.
Once more, the condition checks for project type have been updated from == to =. Consider reverting these changes for readability.
- if [ "$DP_PROJECT_TYPE" = "project_module" ]; then
+ if [ "$DP_PROJECT_TYPE" == "project_module" ]; then
- elif [ "$DP_PROJECT_TYPE" = "project_theme" ]; then
+ elif [ "$DP_PROJECT_TYPE" == "project_theme" ]; then
.gitpod/drupal/drupalpod-setup.sh
Outdated
| if [ "$DP_PROJECT_TYPE" = "project_module" ]; then | ||
| cd "${GITPOD_REPO_ROOT}" && ddev drush en -y "$DP_PROJECT_NAME" | ||
| fi | ||
|
|
||
| # Enable the requested theme | ||
| if [ "$DP_PROJECT_TYPE" == "project_theme" ]; then | ||
| if [ "$DP_PROJECT_TYPE" = "project_theme" ]; then | ||
| cd "${GITPOD_REPO_ROOT}" && ddev drush then -y "$DP_PROJECT_NAME" | ||
| cd "${GITPOD_REPO_ROOT}" && ddev drush config-set -y system.theme default "$DP_PROJECT_NAME" |
There was a problem hiding this comment.
Yet again, the condition checks for project type have been updated from == to =. Consider reverting these changes for readability.
- if [ "$DP_PROJECT_TYPE" = "project_module" ]; then
+ if [ "$DP_PROJECT_TYPE" == "project_module" ]; then
- if [ "$DP_PROJECT_TYPE" = "project_theme" ]; then
+ if [ "$DP_PROJECT_TYPE" == "project_theme" ]; then
.gitpod/drupal/drupalpod-setup.sh
Outdated
| if [ "$DP_PROJECT_TYPE" = "project_core" ]; then | ||
| # Patch the scaffold index.php and update.php files | ||
| # See https://www.drupal.org/project/drupal/issues/3188703 | ||
| # See https://www.drupal.org/project/drupal/issues/1792310 |
There was a problem hiding this comment.
Lastly, the condition check for project type has been updated from == to =. Consider reverting this change for readability.
- if [ "$DP_PROJECT_TYPE" = "project_core" ]; then
+ if [ "$DP_PROJECT_TYPE" == "project_core" ]; then
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- .gitpod/drupal/drupalpod-setup.sh (4 hunks)
Additional comments (Suppressed): 6
.gitpod/drupal/drupalpod-setup.sh (6)
2-2: The use of
set -eu -o pipefailis a good practice for error handling in bash scripts. It ensures that the script will exit if any command returns a non-zero status (due to an error), or if any part of a pipeline fails.5-19: Initializing variables with default values using
${VAR:=}syntax is a good practice. This ensures that even if the environment variable is not set, the script won't fail due to unbound variables.26-26: The change from running
ddev startin the background to direct execution simplifies the script and removes the need for waiting for its completion. However, this might increase the total execution time of the script as it now waits forddev startto complete before proceeding.149-151: Creating temporary composer files and then moving them to the root directory is a good way to ensure that the necessary files are created even if the composer create command fails for some reason. The removal of the temporary directory after copying the files is also a good practice to clean up unnecessary files.
170-172: The use of the
--no-pluginsflag withddev composer configcommands is a good practice. It prevents any plugins from running during the command execution, which can help avoid potential issues caused by those plugins.258-259: The addition of the
--no-installflag to theddev . composer requirecommand is a good practice. This flag tells Composer not to install the package immediately after requiring it, which can be useful in certain scenarios where you want to modify the composer.json file before running the installation.
.gitpod/drupal/drupalpod-setup.sh
Outdated
| if [ -n "$DP_PATCH_FILE" ]; then | ||
| echo found patch file | ||
| echo Applying selected patch "$DP_PATCH_FILE" | ||
| cd "${WORK_DIR}" && curl "$DP_PATCH_FILE" | patch -p1 | ||
| else | ||
| echo did not find | ||
| fi |
There was a problem hiding this comment.
The conditional check for the existence of a patch file and applying it if found is a good practice. However, the echo statements used for debugging purposes ("found patch file", "did not find") should be removed or replaced with more descriptive messages.
echo found patch file
echo Applying selected patch "$DP_PATCH_FILE"
cd "${WORK_DIR}" && curl "$DP_PATCH_FILE" | patch -p1
else
echo did not findto
echo "Patch file found. Applying..."
echo "Applying selected patch $DP_PATCH_FILE"
cd "${WORK_DIR}" && curl "$DP_PATCH_FILE" | patch -p1
else
echo "No patch file found."
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- .gitpod/drupal/drupalpod-setup.sh (4 hunks)
Additional comments (Suppressed): 6
.gitpod/drupal/drupalpod-setup.sh (6)
1-33: The new hunk introduces stricter error handling with
set -eu -o pipefailand initializes variables with default values using${VAR:=}syntax. This is a good practice as it ensures that the script will exit if any command fails or if an undefined variable is used, improving the robustness of the script. The loading of environment variables from.envfile has been removed, which might affect the behavior of the script if these variables were being used elsewhere in the codebase. Please verify this change.26-26: The background execution of
ddev starthas been replaced with a direct call, removing the need for waiting for its completion. This simplifies the script and makes it easier to understand. However, please ensure that this does not introduce any race conditions or timing issues in the rest of the script.149-150: The creation of composer.json and composer.lock files now happens directly after the case statement, without waiting for
ddev startto complete. This change simplifies the script and should not cause any issues as long asddev startdoes not need to complete before these files are created.167-169: The
--no-pluginsflag has been added to theddev composer configcommands. This flag disables all plugins for the command, which can improve performance and prevent potential issues caused by plugins. However, please verify that no necessary plugins are disabled by this change.255-259: The
--no-installflag has been added to theddev . composer requirecommands. This flag prevents Composer from installing the package immediately, which can be useful if you want to modify some files before the installation. However, please verify that the packages are installed later in the script or that the installation is not necessary.270-270: The
ddev drush sicommand has been prefixed withtime, which will print the time it takes to execute the command. This can be useful for performance monitoring and optimization. However, please ensure that the output of thetimecommand does not interfere with any other parts of the script that might be expecting the output ofddev drush si.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- .gitpod/drupal/drupalpod-setup.sh (4 hunks)
Additional comments (Suppressed): 7
.gitpod/drupal/drupalpod-setup.sh (7)
2-2: The use of
set -eu -o pipefailis a good practice for stricter error handling in bash scripts. It ensures that the script will exit if any command returns a non-zero status (due to an error) or if any part of a pipeline fails.5-14: Initializing all variables with empty strings if they do not exist is a good practice to avoid potential issues with unset variables later in the script.
26-26: The change from running
ddev startin the background and waiting for its completion to directly calling it simplifies the script and makes it easier to understand. However, ensure that this does not affect other parts of the script that might rely onddev startrunning in the background.149-150: The creation of required composer.json and composer.lock files has been moved outside the case statement. This change seems logical as these files are required regardless of the project type. However, verify that this does not affect the setup process for different project types.
167-169: The addition of
--no-pluginsflag to thecomposer configcommands improves performance by disabling plugins during the execution of the command. This is especially beneficial in a CI/CD environment where plugins are often not needed.253-256: The conditional check for
$COMPOSER_DRUPAL_LENIENTand subsequent commands have been updated to include the--no-installflag. This prevents Composer from installing the package immediately, which can improve performance by delaying the installation until it's actually needed. However, make sure that the package is installed at some point later in the script.267-267: The use of
ddev config --autois a good practice as it automatically updatessettings.phpand generatessettings.ddev.php. This reduces the need for manual configuration and makes the setup process more efficient.
Rate Limit ExceededYou have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 3 minutes and 45 seconds before requesting another review. To re-trigger a review, convert this PR to a draft and then mark it as ready for review again. Alternatively, you can push a new commit to this PR. |
Summary by CodeRabbit
set -eu -o pipefail.ddev startand removed background execution..envfile.--no-installflag.settings.phpand generation ofsettings.ddev.php.