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

ACC-138: Deploy PLF docker in acceptance #64

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

ACC-138: Deploy PLF docker in acceptance #64

wants to merge 8 commits into from

Conversation

bourasom
Copy link
Contributor

No description provided.

Copy link
Contributor

@vsellier vsellier left a comment

Choose a reason for hiding this comment

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

First cosmetic only review

_functions.sh Outdated Show resolved Hide resolved
}

do_docker_server_deploy(){
# We search the server directory
Copy link
Contributor

Choose a reason for hiding this comment

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

looks a lot of duplicated code just to avoid 2 mkdirs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The duplicated code is here to do separation between the classical and the docker deployment. It is here to do not broke something on the existent code

Comment on lines +1606 to +1619
do_load_deployment_descriptor
do_start_onlyoffice
do_start_ldap
do_start_cmis
do_start_database
do_start_es
do_start_chat_server

# We need this variable for the setenv
export DEPLOYMENT_CHAT_SERVER_PORT

# We need this variable for the setenv
export DEPLOYMENT_ONLYOFFICE_HTTP_PORT
export DEPLOYMENT_CMIS_HTTP_PORT
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be possible this duplicated part here and on the do_default_start by keeping it on the do_start method

@@ -1647,6 +1807,52 @@ do_undeploy() {
fi
}

do_docker_undeploy() {
if [ ! -e "${ADT_CONF_DIR}/${INSTANCE_KEY}.${ACCEPTANCE_HOST}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, it the duplicate code should be avoid as far as possible

Comment on lines +57 to +58
env_var LDAP_HOST "ldap"
env_var DEPLOYMENT_LDAP_PORT "389"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it normal these values are hardcoded ?

echo_info "Exo wasn't deployed in docker, skiping exo docker startup"
return
fi
mkdir -p ${DEPLOYMENT_DIR}/logs/exo
Copy link
Contributor

Choose a reason for hiding this comment

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

The owner or the permissions of the directory have to be adapted to let the exo user inside the container to write

DOCKER_ARGS="${DOCKER_ARGS} -e EXO_JMX_RMI_SERVER_PORT=10002"
DOCKER_ARGS="${DOCKER_ARGS} -e EXO_JMX_RMI_SERVER_HOSTNAME=${DEPLOYMENT_EXT_HOST}"
DOCKER_ARGS="${DOCKER_ARGS} -v ${DEPLOYMENT_EXO_DOCKER_CONTAINER_NAME}_data:/srv/exo:rw"
DOCKER_ARGS="${DOCKER_ARGS} -v ${DEPLOYMENT_DIR}/logs/exo:/var/log/exo:rw"
Copy link
Contributor

Choose a reason for hiding this comment

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

why not using only ${DEPLOYMENT_DIR}/logs/ ? the link to the logs will be broken if a new level of directory is added

_functions_exo_docker.sh Outdated Show resolved Hide resolved
_functions_exo_docker.sh Outdated Show resolved Hide resolved
@@ -152,7 +152,8 @@ do_configure_tomcat_jod() {

do_configure_tomcat_ldap() {
if [ "${DEPLOYMENT_LDAP_ENABLED}" == "true" ]; then
echo_info "Start Deploying Directory ${USER_DIRECTORY} conf ..."
echo_info "Start Deploying Directory ${USER_DIRECTORY} conf ..."
env_var LDAP_HOST "localhost"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why hardcoding this value ?

bourasom and others added 3 commits January 10, 2020 15:02
Co-Authored-By: Vincent Sellier <vsellier@exoplatform.com>
Co-Authored-By: Vincent Sellier <vsellier@exoplatform.com>
Co-Authored-By: Vincent Sellier <vsellier@exoplatform.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants