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

feat: add phpMyAdmin service configuration #801

Merged
merged 10 commits into from
Nov 22, 2024

Conversation

monteshot
Copy link
Contributor

  • Add docker-compose.phpmyadmin.yml for phpMyAdmin service
  • Update svc.cmd to conditionally include phpMyAdmin configuration
  • Ensure phpMyAdmin config file is generated if not present
  • Add regeneratePMAConfig function to core.sh
  • Call regeneratePMAConfig on env changes in env.cmd
  • Include phpMyAdmin as a global service in core.sh

- Add docker-compose.phpmyadmin.yml for phpMyAdmin service
- Update svc.cmd to conditionally include phpMyAdmin configuration
- Ensure phpMyAdmin config file is generated if not present
- Add regeneratePMAConfig function to core.sh
- Call regeneratePMAConfig on env changes in env.cmd
- Include phpMyAdmin as a global service in core.sh
@monteshot
Copy link
Contributor Author

image

@bap14
Copy link
Member

bap14 commented Sep 16, 2024

@monteshot Thanks for the contribution. I would like to suggest that you add a feature flag to this, similar to that of Portainer and DNSMasq, so that those that use external tools like DBeaver or TablePlus can opt-out of running PMA.

I should clarify. I see that you have a flag, but we're still doing unnecessary regeneration if the flag is disabled.

@poespas

This comment was marked as resolved.

- Add WARDEN_PHPMYADMIN_ENABLE variable to install.cmd
- Modify svc.cmd to check WARDEN_PHPMYADMIN_ENABLE
- Update core.sh to regenerate phpMyAdmin config when enabled
@monteshot
Copy link
Contributor Author

@bap14 Hello. Please review the updated PR.

@poespas Hi. If you use PMA by extending the warden-env.yml file in the project directory, you will have two separate PMA apps. Regarding the file in the Warden home directory, this file only needs to give information about db containers(as mentioned in PMA documentation).

…_ENABLE

Add check to ensure WARDEN_HOME_DIR/.env exists and contains
WARDEN_PHPMYADMIN_ENABLE. Default WARDEN_PHPMYADMIN_ENABLE to 1 if
missing. Indent echo statement for consistency.
@navarr
Copy link
Member

navarr commented Sep 23, 2024

Looking over the code, I don't have any concerns. I think this is ready for testing and if all is working correctly - a merge!

@monteshot
Copy link
Contributor Author

Updated the PR with the latest changes from the main branch.
Additionally, I added one more DB to the hidden database list.

@bap14 @navarr, please review and merge the PR when you are ready.
🤝

@@ -55,3 +55,45 @@ function disconnectPeeredServices {
(docker network disconnect "$1" ${svc} 2>&1| grep -v 'is not connected') || true
done
}
function regeneratePMAConfig() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please fix formatting for this function to be the same as in others?

utils/core.sh Outdated
Comment on lines 58 to 99
function regeneratePMAConfig() {

if [[ -f "${WARDEN_HOME_DIR}/.env" ]]; then
# Recheck PMA since old versions of .env may not have WARDEN_PHPMYADMIN_ENABLE setting
eval "$(grep "^WARDEN_PHPMYADMIN_ENABLE" "${WARDEN_HOME_DIR}/.env")"
WARDEN_PHPMYADMIN_ENABLE="${WARDEN_PHPMYADMIN_ENABLE:-1}"
fi

if [[ "${WARDEN_PHPMYADMIN_ENABLE}" == 1 ]]; then
echo "Regenerating phpMyAdmin configuration..."
## generate phpmyadmin connection configuration
pma_config_file="${WARDEN_HOME_DIR}/etc/phpmyadmin/config.user.inc.php"

cat > "${pma_config_file}" <<-EOL
<?php
\$i = 1;
EOL

for container_id in $(docker ps -q --filter "name=mysql" --filter "name=mariadb" --filter "name=db"); do
container_name=$(docker inspect --format '{{.Name}}' "${container_id}" | sed 's#^/##')
container_ip=$(docker inspect --format '{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}' "${container_id}")
MYSQL_ROOT_PASSWORD=$(docker exec "${container_id}" printenv | grep MYSQL_ROOT_PASSWORD | awk -F '=' '{print $2}')
MYSQL_PASSWORD=$(docker exec "${container_id}" printenv | grep MYSQL_PASSWORD | awk -F '=' '{print $2}')
cat >> "${pma_config_file}" <<-EOT
\$cfg['Servers'][\$i]['host'] = '${container_ip}';
\$cfg['Servers'][\$i]['auth_type'] = 'config';
\$cfg['Servers'][\$i]['user'] = 'root';
\$cfg['Servers'][\$i]['password'] = '${MYSQL_ROOT_PASSWORD}';
\$cfg['Servers'][\$i]['AllowNoPassword'] = true;
\$cfg['Servers'][\$i]['hide_db'] = '(information_schema|performance_schema|mysql)';
\$cfg['Servers'][\$i]['verbose'] = '${container_name}';
\$i++;
EOT
done

cat >> "${pma_config_file}" <<-EOT
?>
EOT

echo "phpMyAdmin configuration regenerated."
fi
}
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH this function look quite complicated to read.
I suggest doing following improvements:

  1. Format the function in the same way as others
  2. Is there any reason why we add"?>" to the end of file?
  3. Simlify logic with multi-line sections (EOT, EOL, cat) to something like this a
{ 
   # ...
   echo 'my line 1';
   echo 'my line 2';
} > myfile
Suggested change
function regeneratePMAConfig() {
if [[ -f "${WARDEN_HOME_DIR}/.env" ]]; then
# Recheck PMA since old versions of .env may not have WARDEN_PHPMYADMIN_ENABLE setting
eval "$(grep "^WARDEN_PHPMYADMIN_ENABLE" "${WARDEN_HOME_DIR}/.env")"
WARDEN_PHPMYADMIN_ENABLE="${WARDEN_PHPMYADMIN_ENABLE:-1}"
fi
if [[ "${WARDEN_PHPMYADMIN_ENABLE}" == 1 ]]; then
echo "Regenerating phpMyAdmin configuration..."
## generate phpmyadmin connection configuration
pma_config_file="${WARDEN_HOME_DIR}/etc/phpmyadmin/config.user.inc.php"
cat > "${pma_config_file}" <<-EOL
<?php
\$i = 1;
EOL
for container_id in $(docker ps -q --filter "name=mysql" --filter "name=mariadb" --filter "name=db"); do
container_name=$(docker inspect --format '{{.Name}}' "${container_id}" | sed 's#^/##')
container_ip=$(docker inspect --format '{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}' "${container_id}")
MYSQL_ROOT_PASSWORD=$(docker exec "${container_id}" printenv | grep MYSQL_ROOT_PASSWORD | awk -F '=' '{print $2}')
MYSQL_PASSWORD=$(docker exec "${container_id}" printenv | grep MYSQL_PASSWORD | awk -F '=' '{print $2}')
cat >> "${pma_config_file}" <<-EOT
\$cfg['Servers'][\$i]['host'] = '${container_ip}';
\$cfg['Servers'][\$i]['auth_type'] = 'config';
\$cfg['Servers'][\$i]['user'] = 'root';
\$cfg['Servers'][\$i]['password'] = '${MYSQL_ROOT_PASSWORD}';
\$cfg['Servers'][\$i]['AllowNoPassword'] = true;
\$cfg['Servers'][\$i]['hide_db'] = '(information_schema|performance_schema|mysql)';
\$cfg['Servers'][\$i]['verbose'] = '${container_name}';
\$i++;
EOT
done
cat >> "${pma_config_file}" <<-EOT
?>
EOT
echo "phpMyAdmin configuration regenerated."
fi
}
function regeneratePMAConfig() {
if [[ -f "${WARDEN_HOME_DIR}/.env" ]]; then
# Recheck PMA since old versions of .env may not have WARDEN_PHPMYADMIN_ENABLE setting
eval "$(grep "^WARDEN_PHPMYADMIN_ENABLE" "${WARDEN_HOME_DIR}/.env")"
WARDEN_PHPMYADMIN_ENABLE="${WARDEN_PHPMYADMIN_ENABLE:-1}"
fi
if [[ "${WARDEN_PHPMYADMIN_ENABLE}" == 1 ]]; then
echo "Regenerating phpMyAdmin configuration..."
pma_config_file="${WARDEN_HOME_DIR}/etc/phpmyadmin/config.user.inc.php"
{
echo "<?php"
echo "\$i = 1;"
for container_id in $(docker ps -q --filter "name=mysql" --filter "name=mariadb" --filter "name=db"); do
container_name=$(docker inspect --format '{{.Name}}' "${container_id}" | sed 's#^/##')
container_ip=$(docker inspect --format '{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}' "${container_id}")
MYSQL_ROOT_PASSWORD=$(docker exec "${container_id}" printenv | grep MYSQL_ROOT_PASSWORD | awk -F '=' '{print $2}')
echo "\$cfg['Servers'][\$i]['host'] = '${container_ip}';"
echo "\$cfg['Servers'][\$i]['auth_type'] = 'config';"
echo "\$cfg['Servers'][\$i]['user'] = 'root';"
echo "\$cfg['Servers'][\$i]['password'] = '${MYSQL_ROOT_PASSWORD}';"
echo "\$cfg['Servers'][\$i]['AllowNoPassword'] = true;"
echo "\$cfg['Servers'][\$i]['hide_db'] = '(information_schema|performance_schema|mysql)';"
echo "\$cfg['Servers'][\$i]['verbose'] = '${container_name}';"
echo "\$i++;"
done
echo "?>"
} > "${pma_config_file}"
echo "phpMyAdmin configuration regenerated."
fi
}

- traefik.http.routers.phpmyadmin.tls=true
- traefik.http.routers.phpmyadmin.rule=Host(`phpmyadmin.${WARDEN_SERVICE_DOMAIN:-warden.test}`)||Host(`phpmyadmin.warden.test`)
- traefik.http.services.phpmyadmin.loadbalancer.server.port=80
restart: ${WARDEN_RESTART_POLICY:-always}
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a new line at the end of all files is a best-practice

Suggested change
restart: ${WARDEN_RESTART_POLICY:-always}
restart: ${WARDEN_RESTART_POLICY:-always}

commands/svc.cmd Outdated
Comment on lines 75 to 79
if [[ "${WARDEN_PHPMYADMIN_ENABLE}" == 1 ]]; then
if [[ ! -f "${WARDEN_HOME_DIR}/etc/phpmyadmin/config.user.inc.php" ]]; then
mkdir -p "${WARDEN_HOME_DIR}/etc/phpmyadmin"
fi
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Could you explain why it can't be in the regeneratePMAConfig function?
  2. Can we simplify it to a single if like this?
Suggested change
if [[ "${WARDEN_PHPMYADMIN_ENABLE}" == 1 ]]; then
if [[ ! -f "${WARDEN_HOME_DIR}/etc/phpmyadmin/config.user.inc.php" ]]; then
mkdir -p "${WARDEN_HOME_DIR}/etc/phpmyadmin"
fi
fi
if [[ "${WARDEN_PHPMYADMIN_ENABLE}" == 1 && \
! -f "${WARDEN_HOME_DIR}/etc/phpmyadmin/config.user.inc.php" ]]; then
mkdir -p "${WARDEN_HOME_DIR}/etc/phpmyadmin"
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The main reason to leave PMA file generation here is faulting to build by docker-compose
  2. ++

Combine the conditions for executing regeneratePMAConfig
into a single if statement for improved readability.
Simplify the conditional logic for creating the phpMyAdmin
configuration directory by merging nested if statements
into a single condition.
Remove unnecessary function parentheses and streamline heredoc usage
for phpMyAdmin configuration generation.
Streamline the generation of phpMyAdmin configuration by using a
single block for echoing PHP script content, reducing redundancy
and improving readability.
@monteshot
Copy link
Contributor Author

@ihor-sviziev Can you review PR again?

@navarr navarr self-assigned this Nov 7, 2024
@navarr navarr added the enhancement New feature or request label Nov 7, 2024
@ihor-sviziev
Copy link
Contributor

@navarr @bap14 are there any reasons not to merge this?

@monteshot
Copy link
Contributor Author

Updated the PR with the latest changes from the main branch.

@bap14 @navarr

@navarr navarr added this pull request to the merge queue Nov 22, 2024
Merged via the queue into wardenenv:main with commit b3630e9 Nov 22, 2024
@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 25, 2024

Today, I installed dev version of Warden, ran warden svc up, and got the following notice:
image

Also, running warden svc config shows an error:
image

@monteshot could you check that?

Note: I reported task for this #823

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants