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

[New PR on phpMyAdmin] #232

Closed
wants to merge 4 commits into from
Closed

[New PR on phpMyAdmin] #232

wants to merge 4 commits into from

Conversation

Ferks-FK
Copy link

@Ferks-FK Ferks-FK commented Aug 15, 2021

New PR only with phpMyAdmin modifications, without all those other changes.

Fixes #194

@@ -0,0 +1,153 @@
#!/bin/bash

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not add || exit to all of the commands you run, just add the following code to the top of the file and it will exit automatically if one of the commands fail.

Suggested change
set -e

echo "* The default directory exists, proceeding with the installation...*"
echo "********************************************************************"
cd /var/www/pterodactyl/public || exit
mkdir -p phpmyadmin && chown www-data.www-data /var/www/pterodactyl/public/phpmyadmin -R
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure www-data works for every operating system like CentOS and Debian based?


echo
echo -n "* Password (phpmyadminuser2021): "
read -r MYSQL_PASS_INPUT
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use password inputs for reading passwords because doing it like this would show the password in the terminal.

echo "******************************"
echo "* Username: $MYSQL_USER"
echo "* Database name: $MYSQL_DATABASE"
echo "* Password: $MYSQL_PASS"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't show the password in the summary.

Suggested change
echo "* Password: $MYSQL_PASS"
echo "* Password: [censored]"

echo "* Creating database..."
mysql -u root -e "CREATE DATABASE ${MYSQL_DATABASE};"

echo "* Grant privileges..."
Copy link
Contributor

Choose a reason for hiding this comment

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

A small typo here:

Suggested change
echo "* Grant privileges..."
echo "* Granting privileges..."

echo
echo "Here it is:"
echo "*********************************************"
openssl rand -base64 32
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use sed to automatically write to the configuration file instead of letting the user configure it by themselves.

summary

continue_install() {
echo "**************************************************"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use correct indent levels for the code in functions.

@Ferks-FK
Copy link
Author

This script was produced and tested using only Ubuntu 20.04 as a base, I don't use other distros, so I didn't bother to do it.

@sinjs
Copy link
Contributor

sinjs commented Aug 18, 2021

This script was produced and tested using only Ubuntu 20.04 as a base, I don't use other distros, so I didn't bother to do it.

Please do bother to do it, since this script is for CentOS 7/8 and Ubuntu 18.04/20.04 and Debian 9/10

@Ferks-FK Ferks-FK closed this Aug 23, 2021
@vilhelmprytz
Copy link
Member

Hello, may I ask why this was closed?

@Ferks-FK
Copy link
Author

Ferks-FK commented Aug 23, 2021

Hello, may I ask why this was closed?

Because apparently my help is not relevant when reading this.

This script was produced and tested using only Ubuntu 20.04 as a base, I don't use other distros, so I didn't bother to do it.

Please do bother to do it, since this script is for CentOS 7/8 and Ubuntu 18.04/20.04 and Debian 9/10

@vilhelmprytz
Copy link
Member

Sorry, I don't think that was the intended message. Your contribution is always appreciated! I can make sure to test it on other distros, but there are other changes suggested. Let us know if you are not able to solve them and I can modify your PR directly. I don't think you have to close it 🙂.

@sinjs
Copy link
Contributor

sinjs commented Aug 24, 2021

Hello, may I ask why this was closed?

Because apparently my help is not relevant when reading this.

This script was produced and tested using only Ubuntu 20.04 as a base, I don't use other distros, so I didn't bother to do it.

Please do bother to do it, since this script is for CentOS 7/8 and Ubuntu 18.04/20.04 and Debian 9/10

With that I meant that you should make the script compatible for the other distros, not that you're not needed.

@Ferks-FK Ferks-FK reopened this Aug 24, 2021
I will need help using SED to automatically paste the information into the configuration file.
@vilhelmprytz
Copy link
Member

Anything new regarding this? I could solve some of the CI linting errors but you have set to not allow edits from the maintainer. Do you wish to continue working on this or should I merge into another branch and see if I can continue on it? 🙂

@ririko5834
Copy link

When this will be added?

@vilhelmprytz
Copy link
Member

There are unsolved problems that need to be changed before we can merge, but this PR does not allow edits from the maintainer (me) so it's up to the submitter 🙂

@Ferks-FK Ferks-FK closed this Dec 16, 2021
@CASPERg267
Copy link

any updates on this feature?

@Ferks-FK
Copy link
Author

Ferks-FK commented Jul 6, 2022

any updates on this feature?

New PR Here

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.

[Suggestion] Add a phpmyadmin install option to the script
5 participants