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

Make auto db creation optional with DB_CREATE #558

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhansen2015
Copy link

Added a DB_CREATE option (default true) to allow skipping the call to bundle exec rake db:create. Particularly important for connecting to databases where the user doesn't have permission to create databases.

Added a `DB_CREATE` option (default `true`) to allow skipping the call to `bundle exec rake db:create`. Particularly important for connecting to databases where the user doesn't have permission to create databases.
@jhansen2015 jhansen2015 changed the title Make auto db creation optional with DB_CREATE #557 Make auto db creation optional with DB_CREATE May 27, 2024
@jhansen2015 jhansen2015 changed the title #557 Make auto db creation optional with DB_CREATE Make auto db creation optional with DB_CREATE May 27, 2024
@jhansen2015
Copy link
Author

A way to address #557.

Copy link
Collaborator

@jcormier jcormier left a comment

Choose a reason for hiding this comment

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

Sorry it took so long to look at this

DB_CREATE=false
-----------------------
EOM
return $res
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be an exit if we are to keep the previous behavior. As is, this will skip db migration and then redmine will continue to try to start, likely leading to more confusing errors.

@@ -1048,8 +1048,27 @@ migrate_database() {
CACHE_VERSION=
[[ -f ${REDMINE_DATA_DIR}/tmp/VERSION ]] && CACHE_VERSION=$(cat ${REDMINE_DATA_DIR}/tmp/VERSION)
if [[ ${REDMINE_VERSION} != ${CACHE_VERSION} ]]; then
if [[ "${DB_CREATE:-true}" == "true" ]] ; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

defaults should all be set in assets/runtime/env-defaults for consistency

Comment on lines +1053 to +1060
# Disable "exit on error" so we can capture the error code
set +e
exec_as_redmine bundle exec rake db:create >/dev/null
res=$?
# Restore "exit on error"
set -e

if [[ 0 -ne $res ]] ; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note shellcheck will tell you its better to if check the command directly instead of storing the results. I think this also would mean we don't have to disable exit on error.

Suggested change
# Disable "exit on error" so we can capture the error code
set +e
exec_as_redmine bundle exec rake db:create >/dev/null
res=$?
# Restore "exit on error"
set -e
if [[ 0 -ne $res ]] ; then
if ! exec_as_redmine bundle exec rake db:create >/dev/null
then

Copy link
Author

Choose a reason for hiding this comment

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

Note shellcheck will tell you its better to if check the command directly instead of storing the results. I think this also would mean we don't have to disable exit on error.

I wanted to retain the exit value from rake db:create, since I don't know the range of non-zero values that it can produce. The only way I know of doing that is to do the res=$?, which requires disabling "exit-on-error".

I'm happy to change it you are ok with returning a fixed value, e.g.:

if ! exec_as_redmine bundle exec rake db:create >/dev/null ; then
  echo <<-EOM
Failed to create db. If db exists, and user (or role) exists and has access to db but no permission to create databases, set:
-----------------------
DB_CREATE=false
-----------------------
EOM

       # FWIW, using "return" here would be consistent a similar use of "exec_as_redmine' in "redmine_check_database_connection"
        exit 1
fi

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.

None yet

2 participants