Skip to content
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

fix: Use explicit paths in install.yaml instead of assuming pwd #126

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rfay
Copy link
Member

@rfay rfay commented Jul 30, 2024

The Issue

The install.yaml makes many assumptions about what directory we're in in pre_install_hooks, post_install_hooks, and yaml_read_files

Except... yaml_read_files cannot currently be made explicit.

How This PR Solves The Issue

Make those all explicit.

Manual Testing Instructions

Test with

ddev get https://github.com/rfay/ddev-platformsh/tarball/20240730_explicit_paths_install_yaml

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

@rfay rfay requested a review from stasadev July 30, 2024 17:44
drupal9)
mkdir -p .drush
drupal*)
mkdir -p ${DDEV_APPROOT}/.drush
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mkdir -p ${DDEV_APPROOT}/.drush
mkdir -p ${DDEV_APPROOT}/.ddev/.drush

@@ -71,6 +71,7 @@ pre_install_actions:
read platform_project
echo "platform_project = '${platform_project}'"
# Put the platform_project in to the project's web environment
cd ${DDEV_APPROOT}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it would not make impact on ddev config. So more add-ons will be affected.

Given this, maybe changing the pre_install_actions context to .ddev is too disruptive?

(There is one more ddev config inside this yaml, that call ddev config, that you didn't change.)

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cd ${DDEV_APPROOT} can be removed, and I like the more explicit changes in post_install_actions, it's always hard to guess what directory you're in without the full path.

@@ -71,6 +71,7 @@ pre_install_actions:
read platform_project
echo "platform_project = '${platform_project}'"
# Put the platform_project in to the project's web environment
cd ${DDEV_APPROOT}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cd ${DDEV_APPROOT}

@@ -83,6 +84,7 @@ pre_install_actions:
#ddev-description:Setting PLATFORM_APPLICATION_NAME
if !( {{ contains "PLATFORM_APPLICATION_NAME" (list .DdevProjectConfig.web_environment | toString) }} ); then
# Put the platform_project in to the project's web environment
cd ${DDEV_APPROOT}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cd ${DDEV_APPROOT}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants