Templatize and refactor 5.7+ entrypoint scripts

[ltangvald]

Note: May want to split this up, but putting everything in a single request for any discussions that may arise.

Unifies the entrypoint scripts for 5.7 and 8.0, since they do not need to be different at all, and adds them to a debian template that is copied into each directory by update.sh

Also refactors the entrypoint script by moving logic for starting and stopping the server into functions and adding timestamps and error levels to the echo lines.

Closes #490

[tianon]

There's really not a whole lot that's different between these and 5.5/5.6 -- do you think it'd make sense to do some runtime detection and switch behavior in the scripts dynamically in order to hit all four versions? We could also have this script include the nuances for the lower versions and have it dynamically update the template when copying it to the lower versions.

[ltangvald]

Yeah, you're right it's not that much work (we do this at https://github.com/mysql/mysql-docker/blob/mysql-server/gen_dockerfiles.sh).

[tianon]

@ltangvald is there anything @yosifkit and I can do to help you move this forward and/or support you better? 😄

[ltangvald]

Right now it's hanging on me, so only if you can find some more hours in the day :D
But I have some time this week, so will update the PR with the suggestions we've discussed and push it :)

[ltangvald]

Commits should resolve all conversations except the one about adding 5.5 and 5.6 to the template. We have this upstream, but the implementation results in chunks of dead code in the entrypoint scripts of the newer versions: https://github.com/mysql/mysql-docker/blob/mysql-server/5.7/docker-entrypoint.sh#L89.
Do you have any ideas for how to better resolve this? The problem is mostly just the non-daemonize method for starting the server.

I also added one change for storing the root password in a file instead of on the command line, since mysqladmin also needs it.

[tianon]

So this is ready for review again? Do you want us to wait while you do more work on it (such as consuming #448, MariaDB/mariadb-docker#183, etc)? (Wanting to move this forward if we can 💪)

[ltangvald]

@tianon: Sorry, yes, it was meant to be review-ready :)
However, I also added a "docker" prefix to the function names now (from the discussion at docker-library/postgres#496) plus a small typo fix.

We could add the other issues, but they could just as well be done after, and the templating should make future fixes easier to handle

[ltangvald]

@yosifkit Good if you also take a look at the changes :)

[glensc]

why not move this block to subroutine as well?

[yosifkit]

My thoughts are along the lines that maybe this doesn't go far enough with functionalizing the entrypoint. This could be separate discussion, but I think if we are going to create functions in the entrypoint that users can use from initidb.d scripts, then we might as well go all the way to providing a consistent well-reasoned interface.

If docker-library/postgres#496 seems reasonable, I'd like to get MySQL (and mariadb + percona) as similar as possible. I am definitely open to suggestions for improvement there; especially if it will make it work better for MySQL. This would allow users more places to hook in customization by sourcing the entrypoint script with their own script without us having to explicitly support more environment variables, an always run initidb.d directory (docker-library/percona#58), or an only run initdb.d (#357, #423).

Where can we collaborate to bring to two ideas closer together? Would defining the function list (and possible arguments) be a good place to start? Here is what i have so far in the postgres rework:

file_env
docker_create_database_dirs
docker_init_database_dir
docker_print_password_warning
docker_process_init_files
# maybe drop the `p`, or use docker_process_sql or something
docker_psql_run
docker_setup_database
docker_setup_env_vars
# there isn't really a MySQL equivalent of this one, so it should probably be unique to postgres
docker_setup_pg_hba
# perhaps these should be a more generic "docker_temporary_sqlserver_start"
docker_temporary_pgserver_start
docker_temporary_pgserver_stop

[tianon]

Friendly ping @ltangvald ❤️

[ltangvald]

@tianon Working on moving as much logic as possible into functions, as yosifkit suggests :)

[ltangvald]

@tianon @yosifkit I've refactored further to put as much logic as possible into functions. The list is a bit different, but could you look it over and maybe suggest refinements to bring it closer to pg?

[glensc]

@ltangvald there's still rather large block in global scope:

mysql/.template.Debian/docker-entrypoint.sh

Lines 270 to 318 in 94616c4

	# If this is true then there's no database, and it needs to be initialized
	if [ ! -d "$DATADIR/mysql" ]; then
	docker_verify_env
	docker_init_database_dir "$@"
	docker_init_client_command
	docker_note "Starting temporary server"
	docker_start_server "$@"
	# For 5.7+ the server is ready for use as soon as startup command unblocks
	if [ "${MYSQL_MAJOR}" = "5.5" ] || [ "${MYSQL_MAJOR}" = "5.6" ]; then
	docker_note "Waiting for server startup"
	docker_wait_for_server "${mysql[@]}"
	fi
	docker_note "Temporary server started."
	if [ -z "$MYSQL_INITDB_SKIP_TZINFO" ]; then
	docker_load_tzinfo
	fi
	if [ ! -z "$MYSQL_RANDOM_ROOT_PASSWORD" ]; then
	docker_generate_root_password
	fi
	docker_init_root_user
	docker_write_password_file
	docker_init_database_user
	echo
	for f in /docker-entrypoint-initdb.d/*; do
	docker_process_init_file "$f" "${mysql[@]}"
	done
	if [ ! -z "$MYSQL_ONETIME_PASSWORD" ]; then
	docker_expire_root_user
	fi
	docker_note "Stopping temporary server"
	docker_stop_server
	docker_note "Temporary server stopped"
	# Remove the password file now that initialization is complete
	rm -f "${PASSFILE}"
	unset PASSFILE
	echo
	docker_note "MySQL init process done. Ready for start up."
	echo
	fi

why not move that into function as well?

[ltangvald]

That block is (almost) the entire script; Overriding it essentially means overriding the entire entrypoint.
But maybe it would be useful to move all the helper functions into a separate file so they would be available for someone who wants to define their own behavior from scratch.

[glensc]

imho the only goal should not be override possibility, but code readability too. moving to function is more compact and allows using local variables.

with that in mind, i tend to write my shell scripts nowadays as:

#!/bin/sh

func() {
  ...
}

main() {
  local foo=$1 bar
  func bar
}

main "$@"

• 1

[ltangvald]

Yeah, that's a good point. I've added a docker_main function now (there was also a bug with trying to run anything other than mysqld, as it would still try to fetch server config)

About local variables: I'm on somewhat shaky ground when it comes to bash scoping, but right now the script relies on global variables, e.g. docker_init_env sets DATADIR and SOCKET, which are used in numerous places.

[yosifkit]

I thought about moving the helper functions to another file for postgres, but instead I just hid the main logic in a function and only run that function when the file is not sourced so that a user can source our one file and then choose which pieces they want to run. That way we don't have to worry about telling users about a second file that they can source to get our init steps.

# check to see if this file is being run or sourced from another script
 _is_sourced() {
 	# https://unix.stackexchange.com/a/215279
	[ "${FUNCNAME[${#FUNCNAME[@]} - 1]}" == 'source' ]
}
_main() {
	# init logic
}

if !_is_sourced; then
	_main "$@"
}

Do you think this would be good to do in the MySQL image too?

As for function names and usage, i'm still lining up the MySQL ones with the ones I proposed for PostgreSQL Server so that we can get a unified set (plus custom ones for each).

[glensc]

I like that Pythonish is_main check ;)

I think you're supposed to use [['s with ==, or if you want to be POSIX compatible then ['s go with single =.

[tianon]

I think you're supposed to use [['s with ==, or if you want to be POSIX compatible then ['s go with single =.

It should really drop the == down to just = -- using [[ with == is a Bashism (and is only necessary for glob matching, which is more reliably performed via case xxx in GLOB) ... instead).

(But if we're getting that picky, FUNCNAME isn't acceptable since it's also a Bashism 😆)

[lag-linaro]

Surely, surely, this is ready to go now.

Last, little, push ...

[tianon]

Agreed, let's take this across the finish line and dial in anything else in follow-ups. 👍

(I think we've discussed/evaluated the high level bits enough that we're relatively safe.)

[lag-linaro]

👏