fix makemysql so that easywp might work #154

axmmisaka · 2020-05-11T08:37:32Z

Why kuroko? bc the committer is shirai kuroko (use git log)
Judgement desu no
idk if i have access on this repo so i'll just use mine

#137
Problem is it used to just output sudo -u mysql /opt/share/utils/makeservices/makemysql-real, and after #129 it's nothing, #138 sometimes give a string so this will screw up, as easywp rely on some stuff extracted from tail -n 1 that look like Your MySQL database password is: aioeuwhgviujashfuiasb
This one just simulate if we were running sudo -u mysql /opt/share/utils/makeservices/makemysql-real and output the stuff in the last line so there are probably backwards compatibility

Another feature: use argument --ignore-wp to not reset in case you don't want to, and this shall remain a secret so that it is not abused

Potential problems:

Does wp core is-installed --quiet --path=$HOME/public_html > /dev/null 2>&1 return true if wp exists and false otherwise
Change password if installed with easywp is correct but change password only if installed with easywp is a wrong statement, idk if wp-cli will also handle this
~~2.5 i didn't run make test for style check - ok i screwed up, wait one~~ Now it worked
idk if it works, i can't test

axmmisaka · 2020-05-11T08:49:32Z

Remember to close #103 also after merging this (if this works)
Now CS170 time for me, damn

axmmisaka · 2020-05-11T19:05:20Z

lets see if not running make test cause problems again
it doesn't!!!!! salud

kpengboy · 2020-05-12T04:10:59Z

makeservices/makemysql

+    wp config set DB_PASSWORD "$PASS" > /dev/null 2>&1
+fi
+# Added for backward compatibility - the last line will be
+# extracted by "tail -n 1" and the mysterious "grep"


You may wish to mention which script is running these commands.

kpengboy · 2020-05-12T04:14:58Z

makeservices/makemysql

+# extracted by "tail -n 1" and the mysterious "grep"
+# will extract the password after the colon
+# So that it simulate the same behaviour as the original makemysql
+# which output the same thing as in makemysql-real


# So that it simulate the same behaviour as the original makemysql # which output the same thing as in makemysql-real

I would remove these comments; the comments in the previous lines about easywp already tell me everything I need to know.

cg505 · 2020-05-12T21:25:29Z

I feel like it would make more sense to update the whole stack here in one swipe:

makemysql-real mostly prints to stderr, and only prints the password to stdout, which means we don't have to do the scary grep incantation that's currently in makemysql
makemysql has an option to skip WP config, and also an option to only output the password (instead of the normal, nice message "Your MySQL database password is: ")
easywp uses the options in Reorganize scripts, remove system-only scripts #2 to get the DB password without doing any weird text manipulation.

axmmisaka · 2020-05-13T00:43:48Z

will do

axmmisaka · 2020-05-13T01:17:16Z

3. `easywp` uses the options in #2 to get the DB password without doing any weird text manipulation.

Do u mean 2. here or PR #2...?

cg505 · 2020-05-13T01:19:17Z

Do u mean 2. here or PR #2...?
point 2. here, not the PR. it just autolinked.

cg505

looking pretty good so far

cg505 · 2020-05-13T17:18:38Z

makeservices/easywp

@@ -57,7 +57,7 @@ fi

 # Get SQL password
 echo "Resetting database password..."
-sqlpass=$(echo "yes" | makemysql | tail -n 1 | grep -Po '(?<=: )([0-9a-zA-Z]){24,}$')
+sqlpass=$(makemysql --unreadable-output --silent)


seems like you should have --ignore-wp here too, since there won't be any wordpress installation yet?

--ignore-wp is for those who installed WordPress but don't want to change their pw, here the thing will be done by wp-cli in makemysql imo

cg505 · 2020-05-13T17:19:39Z

makeservices/makemysql

+            YES="--silent"
+            shift
+            ;;
+        # I've seen -*|--*=), but idk what does the = serve


it's for options like --key=value probably, but i wouldn't worry about it

cg505 · 2020-05-13T17:22:47Z

makeservices/makemysql

+    esac
+done
+
+PASS=$(sudo -u mysql /opt/share/utils/makeservices/makemysql-real $SILENT | tail -n 1)


I'm not sure what's the point of --silent, since we're always only going to see the last line (because of tail). Maybe we could make it so that makemysql-real prints its messages to stderr so they aren't affected but the subshell and tail?

That is what I was expecting (only printing the password), but I do not know if other calls in makemysql-real print anything to stdout, so tail was added to make sure.
That being said, I was considering about what if makemysql-real failed, without grep PASS would be some weird stuffs

It seems like we should be adding the same option to control makemysql-real's output that we are adding to makemysql then.

I've actually done that, but I didn't test makemysql-real and the things it call might output some other mysterious stuff ig

cg505 · 2020-05-13T17:23:58Z

makeservices/makemysql

-if (wp core is-installed --quiet --path="$HOME/public_html" > /dev/null 2>&1) &&
-        ([ $# -ne 1 ] || ( [ $# -eq 1 ] && [ ! "$1" = '--ignore-wp' ])); then
-    echo "WordPress installation detected, changing WordPress mysql password for you."
+if (wp core is-installed --quiet --path="$HOME/public_html" > /dev/null 2>&1) && ! $IGNORE_WP ; then


this is kind of weird, it is literally running the commands true or false

lemme do some research, my shell script sucks

I'd check IGNORE_WP first before running a command.

Nit: beware that putting parentheses around a command runs it in a subshell (which is not necessary in this case). Up to you if you want to keep them around or not, though; I don't think it makes too much of a practical difference in this case either.

https://stackoverflow.com/questions/2953646/how-can-i-declare-and-use-boolean-variables-in-a-shell-script

kpengboy

Thanks for working on this. I've left a couple comments.

kpengboy · 2020-05-18T07:57:47Z

makeservices/makemysql

+    esac
+done
+
+PASS=$(sudo -u mysql /opt/share/utils/makeservices/makemysql-real $SILENT | tail -n 1)


It seems like we should be adding the same option to control makemysql-real's output that we are adding to makemysql then.

kpengboy · 2020-05-18T08:07:52Z

makeservices/makemysql

-if (wp core is-installed --quiet --path="$HOME/public_html" > /dev/null 2>&1) &&
-        ([ $# -ne 1 ] || ( [ $# -eq 1 ] && [ ! "$1" = '--ignore-wp' ])); then
-    echo "WordPress installation detected, changing WordPress mysql password for you."
+if (wp core is-installed --quiet --path="$HOME/public_html" > /dev/null 2>&1) && ! $IGNORE_WP ; then


I'd check IGNORE_WP first before running a command.

Nit: beware that putting parentheses around a command runs it in a subshell (which is not necessary in this case). Up to you if you want to keep them around or not, though; I don't think it makes too much of a practical difference in this case either.

https://stackoverflow.com/questions/2953646/how-can-i-declare-and-use-boolean-variables-in-a-shell-script

kpengboy · 2020-05-18T08:10:37Z

makeservices/makemysql-real

@@ -78,43 +80,50 @@ def main():
        # Read config file.
        mysql_host, mysql_root_pw = read_config()

+        # Added a simple and stupid argument parsing so that other scripts can use it without tunneling in yes.
+        if len(sys.argv) > 1 and sys.argv[1] in ['-s', '--silent']:


Personally, I'd use argparse.

There's only one possible argument which I don't want to utilize a heavy parser

kpengboy · 2020-05-18T08:11:57Z

makeservices/makemysql-real


        # Check if the database already exists.
        try:
-            print(">>> Checking if database '{}' already exists...".format(username))
+            if not silent:


if not silent: print(

Probably worth creating a wrapper for this...

kpengboy · 2020-05-18T08:12:29Z

makeservices/easywp

@@ -57,7 +57,7 @@ fi

 # Get SQL password
 echo "Resetting database password..."
-sqlpass=$(echo "yes" | makemysql | tail -n 1 | grep -Po '(?<=: )([0-9a-zA-Z]){24,}$')
+sqlpass=$(makemysql --unreadable-output --silent)


Maybe --machine-readable or similar?

axmmisaka · 2021-03-26T04:31:27Z

Idk what happened to my local repo, but there seems to be a screwup

Updates:

Found a stupid logical mistake I made in makemysql. --machine-readable does not make sense as it's doing the same thing as --silent does, and we always want to makemysql-real silently when calling it in the script - and even if we did not run it silently, it's in a subshell so nothing will be outputted...
Added some exit code check after sub-shelling
Applied cg505 and kpengboy's suggestions on making script more efficient

Can't believe I procrastinated on this for almost 1 year!

kpengboy · 2021-04-17T23:36:25Z

makeservices/easywp

+if [ $? -ne 0 ] || [[ -z "$sqlpass" ]]; then
    echo -e "\033[31mError:\033[00m Could not retrieve database password. Run makemysql to see the issue."
    exit 1
 fi


As it stands, this code is not going to be executed at all if makemysql exits with a non-zero status due to the set -e at the top of the script.

To avoid this...I guess we can add a set +e right before the makemysql call and a set -e after the if block?

kpengboy · 2021-04-18T00:06:42Z

makeservices/makemysql-real

@@ -78,43 +83,45 @@ def main():
        # Read config file.
        mysql_host, mysql_root_pw = read_config()

+        # Added a simple and stupid argument parsing so that other scripts can use it without tunneling in yes.
+        if len(sys.argv) > 1 and sys.argv[1] in ['-s', '--silent']:


I would call it --quiet if I were you. --silent sounds like no output would be printed at all.

Changed names

kpengboy · 2021-04-18T00:13:29Z

makeservices/makemysql


 # Check if wp is installed with wp-cli
 # And change password if installed with easywp
 # But change password only if installed with easywp is a wrong statement
 # Use --ignore-wp flag to skip this process
-if (wp core is-installed --quiet --path="$HOME/public_html" > /dev/null 2>&1) && ! $IGNORE_WP ; then
+if $IGNORE_WP && ! wp core is-installed --quiet --path="$HOME/public_html" > /dev/null 2>&1 ; then


Shouldn't it be ! $IGNORE_WP?

Yes, I was stupid!

kpengboy · 2021-04-18T01:13:51Z

makeservices/makemysql

+    # Added for backward compatibility - the last line will be
+    # extracted by "tail -n 1" and the mysterious "grep"
+    # will extract the password after the colon in legacy codes
+    # So that it simulate the same behaviour as the original makemysql
+    # which output the same thing as in makemysql-real


I think this is out of date?

redirected some stuff to stderr fix stupid mistakes for makemysql-real

…and make things fancy but they might not work as I didn't test it

…did not run pre commit yet

1. Changed --silent to --quiet 2. Disable `set -e` at places where error-handling exists 3. Added some more instructions 4. Removed some redundant stuff, but idk if this will blow stuff up

axmmisaka · 2021-05-27T20:15:07Z

oh no, i decided to rebase and force-push, seems like a very bad decision

Anyway, @kpengboy 's valuable suggestions are applied, and the only thing remaining is pre-commit and style check... Which I need to do on supernova

emmatyping · 2021-09-24T00:54:46Z

Would you be interested in reviving this PR?

axmmisaka · 2021-09-24T02:59:31Z

Yes, I think it's good tho?

emmatyping · 2021-09-24T05:50:24Z

It appears CI is failing, could you look into that? Also wasn't sure if there were any review comments left to do

kpengboy · 2021-09-24T08:18:47Z

It seems that you'll need to fix some shellcheck issues? Otherwise LGTM, assuming that you have tested this.

axmmisaka · 2021-09-30T06:17:53Z

Remember to SQUASH if you reviewed and decided to merge please 😴

kpengboy

LGTM, assuming you have tested these changes thoroughly.

axmmisaka · 2021-10-19T04:04:42Z

As dev-tsunami does not exist, each of the scripts are tested individually with guser on tsunami.

ja5087 · 2021-10-24T19:05:43Z

makeservices/makemysql

+# last line. The --quiet will also make sure it only output the password...
+PASS=$(sudo -u mysql /opt/share/utils/makeservices/makemysql-real --quiet)
+
+if [ $? ] ; then


should these be if [[ $? -ne 0 ]]? Since we are checking if the return code was abnormal (0 is success)

I see you inverted this in 051e0387350c67f0fd6cf976da25ad8511ad46bb to make precommit pass. What was the issue?

I don't know why but pre-commit say I should use if [ $? ] instead of if [[ $? -ne 0 ]] so I accepted this recommendation

@kpengboy

This commit consists 18 commits in total, earliest one dating back to May 2020, intended to fix chain reactions caused by #129. This commit: 1. Changed `makemysql-real`, allowing it to output only the password and nothing else if a specific argument is given 2. Changed `makemysql`, allowing it to correctly fetch password printed by `makemysql-real`, allowing it to be silent if no wordpress installation is found, allowing it to not change wordpress password if a specific argument is given, and allowing it to output only the password and nothing else if a specific argument is given. 3. Changed `easywp` so that it is compatible with the updated `makemysql`. Hopefully, this will not break ocf infra. 18 commits: * rewrote so that it might work * idk wat autopep8 changed/suggested i followed its advice * forgot to add the messages * Added argument parsing and non-human-friendly output * Squashed two commits redirected some stuff to stderr fix stupid mistakes for makemysql-real * updated all three scripts so that they support some silent arguments and make things fancy but they might not work as I didn't test it * fix stupid bugs made in 3cac9d4 * fix pre-commit problem made in 3cac9d4 * Wrapper for if silent * Fixed a stupid logical mistake and added some stuff in bash scripts; did not run pre commit yet * Applied @kpengboy's suggestions 1. Changed --silent to --quiet 2. Disable `set -e` at places where error-handling exists 3. Added some more instructions 4. Removed some redundant stuff, but idk if this will blow stuff up * Bug: if quite is specified, do not ask if user wants to proceed. * Indentation Errors * Fix, silent should be global variable * Fixed some bugs in easywp * fix comment * fix so precommit pass * more to squash

axmmisaka changed the title ~~rewrote makemysql so that easywp might work~~ fix makemysql so that easywp might work May 11, 2020

kpengboy reviewed May 12, 2020

View reviewed changes

cg505 reviewed May 13, 2020

View reviewed changes

kpengboy reviewed May 18, 2020

View reviewed changes

kpengboy reviewed Apr 18, 2021

View reviewed changes

axmmisaka added 11 commits May 28, 2021 04:09

rewrote so that it might work

3190152

idk wat autopep8 changed/suggested i followed its advice

6596e3a

forgot to add the messages

f2b495c

Added argument parsing and non-human-friendly output

06d1355

Squashed two commits

7cdbb58

redirected some stuff to stderr fix stupid mistakes for makemysql-real

updated all three scripts so that they support some silent arguments …

e43bdb5

…and make things fancy but they might not work as I didn't test it

fix stupid bugs made in 3cac9d4

ea004ec

fix pre-commit problem made in 3cac9d4

9cf60d7

Wrapper for if silent

ef8816f

Fixed a stupid logical mistake and added some stuff in bash scripts; …

5e9b52d

…did not run pre commit yet

Applied @kpengboy's suggestions

2798e6b

1. Changed --silent to --quiet 2. Disable `set -e` at places where error-handling exists 3. Added some more instructions 4. Removed some redundant stuff, but idk if this will blow stuff up

axmmisaka force-pushed the makemysql-kuroko branch from ed928b3 to 2798e6b Compare May 27, 2021 20:11

Bug: if quite is specified, do not ask if user wants to proceed.

6aa0b56

axmmisaka added 6 commits September 29, 2021 19:20

Indentation Errors

7431eef

Fix, silent should be global variable

0c19f5a

Fixed some bugs in easywp

6b7b2c3

fix comment

7fe6f5c

fix so precommit pass

051e038

more to squash

bb20eb4

kpengboy approved these changes Oct 5, 2021

View reviewed changes

axmmisaka merged commit 838baed into ocf:master Oct 19, 2021

emmatyping mentioned this pull request Oct 23, 2021

easywp broken #137

Closed

ja5087 reviewed Oct 24, 2021

View reviewed changes

axmmisaka mentioned this pull request Nov 1, 2021

[OCF/util #137] easywp broken #138

Closed

fix makemysql so that easywp might work #154

fix makemysql so that easywp might work #154

Conversation

axmmisaka commented May 11, 2020 • edited Loading

axmmisaka commented May 11, 2020 • edited Loading

axmmisaka commented May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cg505 commented May 12, 2020

axmmisaka commented May 13, 2020

axmmisaka commented May 13, 2020

cg505 commented May 13, 2020

cg505 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axmmisaka May 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpengboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axmmisaka commented Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axmmisaka commented May 27, 2021

emmatyping commented Sep 24, 2021

axmmisaka commented Sep 24, 2021

emmatyping commented Sep 24, 2021

kpengboy commented Sep 24, 2021

axmmisaka commented Sep 30, 2021

kpengboy left a comment

Choose a reason for hiding this comment

axmmisaka commented Oct 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axmmisaka commented May 11, 2020 •

edited

Loading

axmmisaka commented May 11, 2020 •

edited

Loading

axmmisaka commented May 11, 2020 •

edited

Loading

axmmisaka May 13, 2020 •

edited

Loading

axmmisaka commented Mar 26, 2021 •

edited

Loading