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

Small cleanups #156

Merged
merged 5 commits into from
Feb 12, 2024
Merged

Small cleanups #156

merged 5 commits into from
Feb 12, 2024

Conversation

paulsheldrake
Copy link
Contributor

Removes docker images that don't work on Apple Silicon
Adds a command to install a solr core on our custom solr image
Updates DB and PHP versions
Removes hardcoded values in docksal.env

@paulsheldrake paulsheldrake merged commit 42a3c87 into main Feb 12, 2024
@paulsheldrake paulsheldrake deleted the cleanup branch February 12, 2024 13:57
.docksal/docksal.env Show resolved Hide resolved
.docksal/docksal.yml Show resolved Hide resolved
.docksal/docksal.yml Show resolved Hide resolved
.docksal/docksal.yml Show resolved Hide resolved
@@ -11,6 +11,8 @@ set -e
ENVIRONMENT=${1:-"$hostingenv"}
DOCROOT_PATH="${PROJECT_ROOT}/${DOCROOT}"

fin composer install
Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed for support, but not for build.

Could we add an additional command for support? Something like fin rc which could do composer install, then fin refresh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why can't we just keep this in?

Copy link
Contributor

Choose a reason for hiding this comment

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

It has nothing to do with refeshing the database, and often, I will just need to reset the database, not re-install composer.

How about fin rebuild?
#111

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer not to have a whole new command. There is no technical reason it can't be there and it makes sure when wee pull the DB we can run the site

Copy link
Contributor

Choose a reason for hiding this comment

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

If I run fin init, as users should to reset their environments, composer install is run as part of init-site. Then fin refresh is run. This change you are proposing would cause composer install to run twice.

I see fin rebuild as a good compromise as it is descriptive to what it is actually doing, building composer and refreshing the database, and would fit the needs of both support and build, without changing the way fin refresh has always worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would argue that fin refresh as it's been in the starter is inefficient. fin init should be run as minimally as possible because it's an expensive and time-consuming operation. If you run composer install in the refresh command, we just keep the code updated to date in the current branch. If there are no changes in the composer packages it's super quick, if there are changes, then you've just saved yourself the hassle of wondering why your site isn't working from missing modules. this is the way it's been in support from since we've started using docksal and composer.

we can take the composer install out of init-site and then it would just be run once during setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

It can't be removed from init-site as it is needed to scaffold the site for the following commands:

# Fix file/folder permissions
fix_permissions () {
  echo -e "\n${yellow} ${construction} Making file directories writable. ${construction}${NC}"
  echo -e "${green}${divider}${NC}"
  mkdir -p "${SITEDIR_PATH}/files/private"
  chmod 755 "${SITEDIR_PATH}"
  chmod 755 "${SITEDIR_PATH}/files/private"
}

# Initialize local settings files
init_settings () {
  echo -e "${yellow} ${construction} Copy settings files for local development. ${construction}${NC}"
  echo -e "${green}${divider}${NC}"
  ln -sf "${PROJECT_ROOT}/.docksal/etc/conf/settings.php" "${SITEDIR_PATH}/settings.local.php"
  ln -sf "${PROJECT_ROOT}/.docksal/etc/conf/development.services.yml" "${DOCROOT_PATH}/sites/development.services.yml"
}

And at this point, there may or may not be a database to refresh.

That is why I am suggesting a more composable approach, that the refresh command does exactly what it describes, refreshes the database. The proposed rebuild command would rebuild your composer dependencies and refresh the database.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we setup the devops for the project we hook it into a hosting environment so there will always be a DB. that scenario doesn't happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you're just looking for a fresh db, there is fin pull db command which is already baked into the starter

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

Successfully merging this pull request may close these issues.

2 participants