juanluisbaptiste / docker-otrs

The unofficial Znuny/OTRS Ticketing System docker image
https://www.juanbaptiste.tech/category/otrs
GNU Lesser General Public License v3.0
173 stars 101 forks source link

add new variables to make db more configureable #24

Closed wodka closed 6 years ago

wodka commented 6 years ago

Attempt for #23 - This would introduce several new configuration options:

OTRS_DATABASE = otrs
OTRS_DB_USER = otrs
OTRS_DB_HOST = mariadb

edit: removed settings that are no longer in this pull request

juanluisbaptiste commented 6 years ago

What's the point on using the otrs user for the database setup ? absolutely none, the setup can be done with the root user as is done already, you only need the otrs user to connect to the database from the OTRS container.

I will not merge this PR as it is.

wodka commented 6 years ago

I can change it so the setup is done with the root user again - I missed the create_db call in the beginning - thought I could with this eliminate the requirement to have root access by otrs in any way.

would it be ok for you if I split the installation part in 2 steps:

1) to create the db if it does not exist with the root user 2) to setup the db with the otrs user if no tables are present

this way it would be possible to not require a root user

juanluisbaptiste commented 6 years ago

I don't understand why you want to avoid the root user. It is only used to create the database and load up the schemas, there after the otrs user is used. This is standard way of doing it.

wodka commented 6 years ago

I would like to not give the image root permission, if I create the db and the user before it should not be required. Also not to check if the db is already up.

But if you like I could only add the custom user and db host part

juanluisbaptiste commented 6 years ago

I think you are not understanding how the image works. You need BOTH users, first root to create the database AND the otrs user. Then the otrs application uses the otrs user EXCLUSIVELY (it's hardcoded on the configuration), the root user is used never again. Even with your changes the root user will still exist, so what's the point ?

The only change I can allow is the environment variables to configure a different db server and port. Don't forget to update the documentation too.

wodka commented 6 years ago

I would actually prefer to be able to do the setup without having the root user. Same goes for the "wait_for_db()" function that requires the root user :/ - could we change at least the wait_for_db function to also work without root permissions (perhaps something like https://github.com/vishnubob/wait-for-it)?

update the code right now, will add that to the documentation as well.

juanluisbaptiste commented 6 years ago

You need the root user on wait_for_db to check when it's up. The check it does althought can be changed so it doesn't check the user table. For now I'm not going to implement the wait-for-it script. If you want continue working on the env vars, but no other change I will add for now (I'm also working on OTRS 6 update).

wodka commented 6 years ago

thanks :)

the pull request right now only contains the changes for the 3 vars (OTRS_DATABASE, OTRS_DB_USER, OTRS_DB_HOST) plus the readme.

I removed everything else I did

juanluisbaptiste commented 6 years ago

I will check it now. About the db up check, we can continue to review it, but in a separate issue, to not mix both issues.

wodka commented 6 years ago

updated everything

juanluisbaptiste commented 6 years ago

It looks good, one last thing:

Please apply it to otrs-6_0_x branch as that one will become master in a few days after I finish testing of the OTRS 6 image. Then I can backport this to otrs-5_0_x branch and release a new 5.0.x image with this change when the next OTRS 5.x is released.

The OTRS 6 beta image is at juanluisbaptiste/otrs:6beta to test it.

wodka commented 6 years ago

already did that :) - target is otrs-6_0_x

juanluisbaptiste commented 6 years ago

Ahh yes, I didn't notice that, I'll merge then.

Thanks.

juanluisbaptiste commented 6 years ago

Did you test your code ? there were a couple errors that prevented the container from correctly starting, see the latest commits.

juanluisbaptiste commented 6 years ago

This commit: https://github.com/juanluisbaptiste/docker-otrs/commit/f225280d8663c7335ad83958b31407b63150c62f

juanluisbaptiste commented 6 years ago

Also take a look at https://github.com/juanluisbaptiste/docker-otrs/commit/8953165e4cb706329475605365d0b6bfb6c3b215 for a new wait_for_db implementation that doesn't query a table but uses mysql ping instead.