lucasdiedrich / ojs

Open Journal Systems (OJS) is a journal management and publishing system.
GNU General Public License v3.0
19 stars 64 forks source link

Add dynamic config variables. #4

Closed lucasdiedrich closed 5 years ago

lucasdiedrich commented 6 years ago

Add support for add custom config varaiables for ojs config based on OCS_ prefix.

marcbria commented 5 years ago

I see those variables in the Dockerfile:

    OJS_VERSION="ojs-3_1_1-4"   \
    OJS_CLI_INSTALL="0"         \
    OJS_DB_HOST="localhost"     \
    OJS_DB_USER="ojs"           \
    OJS_DB_PASSWORD="ojs"       \
    OJS_DB_NAME="ojs"           \
    OJS_WEB_CONF="/etc/apache2/conf.d/ojs.conf" \
    OJS_CONF="/var/www/html/config.inc.php" \

But I didn't found any other reference anywhere in Dockerfile or compose or bash scripts. Does it means the variables are announced but not used yet?

I'm asking because as you probably know, PKP developed a few other tools and, at least OMP works like OJS so would be nice avoid references to a specific app (ie: OJS) and use generic names instead as "PKP" or generic as "APP".

So, if you agree, my proposal here will be:

    PKP_VERSION="ojs-3_1_1-4"   \
    PKP_CLI_INSTALL="0"         \
    PKP_DB_HOST="localhost"     \
    PKP_WEB_CONF="/etc/apache2/conf.d/ojs.conf" \
    PKP_CONF="/var/www/html/config.inc.php" \
    MYSQL_USER="ojs"           \
    MYSQL_PASSWORD="ojs"       \
    MYSQL_NAME="ojs"           \

I also sorted the variables to keep app ones in top.

Would be nice using also MYSQLHOST but in [mysql's dockerhub](https://hub.docker.com//mysql) is reported this variable is causing issues.

Thanks a lot for your great work Lucas.

I explained your initiative in the last PKP technical committee and they are waiting for a proposal. (I will write soon and I will send it to you to let you estimate the time you need and decide if you join the battle :-))

Talking about time (or the lack of it) did you took a look about the documentation of the official images? https://docs.docker.com/docker-hub/official_images/

lucasdiedrich commented 5 years ago

Marc,

The OJS_variables was used on cli install script, i was designed so the installation screen wasn't need and the script install using that variables, the main idea was to optimize container first initialization, i don't think this is useful anymore. Someone over PKP/OJS forum said there no is no such thing as cli installation for PKP/OJS so i don't think this feature is going to live anymore.

About this issue, the main idea was to when we have an OJS_ prefix variable it should be verifies at the config file as well and change, but by what i saw using a binded config file is much better than this. So i think this should be aborted somehow.

A proposed changing the variables name to use PKP_ sounds better, but i sugest to use PKP_DB_USER, PKP_DB_PASSWORD and PKP_DB_NAME as well, lets say that we make a postgresql image support, the variable would be same.

About the official images i don't think this is our scope, by my perspective, creating an PKP organization and making it official inside Dockerhub sounds better, i'll look into more detail about this.

Thanks.

marcbria commented 5 years ago

Sorry... I grep into your code and I missed the cli_install script. :-)

I remember the thread about the cli installation in the forum and I think was Alec and Clinton who explained to you OJS didn't include an install script yet and suggested the wget/curl method.

Installing an ojs from the command-line is something that need to be implemented one day so it was added as a feature request in PKP github, but I have the sensation nobody is very much concerned about this.

If you see potential in the cli_install script, please let me know and I will transfer your arguments to PKP.

I recognize it could be a nice addition but in my installations, what I found useful is creating a OJS from a former DB model.

I mean, when you have 20 journals... and you want to create the 21, you know some fields are going to be the same (copyright, support name/mail, lang settings) so for those scenarios I created a bash script called "mojo", that sed's over the mysqldump of the DB to create a new site.

Mojo is outdated right now, but I'm working on "dojo", a new script that superseded mojo because is able to work over docker ojs sites.

So (sorry for the long explanation)... if you don't have cli-install-script, I will suggest creating a model (a fake ojs site with the common info) and use dojo to "clone" future sites, instead of building it all from zero.

About the variable names, your proposal looks great to me.

About the official images, the idea is just what you suggest (PKP organization in dockerhub and so on) but right now nobody in the PKP team knows docker, so this is why I'm asking if you are interested in going on steep further and move from being the maintainer of your Dockerfile to be the maintainer of the official PKP one.

You can be honest. "I'm overwhelmed with other jobs" is a perfectly valid answer. :-)

lucasdiedrich commented 5 years ago

@marcbria , about the cli_install script, i do think its a great idea to bypass the OJS installation page, once we already have the database and others infos on the container initialization there makes no sense having an php installation page.

But, i would like to know more about your installations environments, it seems very different from mine, later i will take a deep look at yours mojo/dojo script.

I'm going to standardize the container variables after i finish the php7 upgrade.

And finally, about the manage the docker environments for pkp, i'm more than happy to be a part of PKP helping and managing the docker tasks. So count me on. I just can't be at full time on it.

Fell free to reach me over Google Hangouts if you so we can talk about this. Thanks for all.

Potomac54 commented 5 years ago

in fact there is already a cli tool installation script in OJS source code :

tools/install.php

https://github.com/pkp/ojs/blob/master/tools/install.php

the script doesn't seems to accept arguments, it works by asking questions to the user

lucasdiedrich commented 5 years ago

Going to close this issue again and fix this over #16 issue;