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-119: allow docker image deployment #34

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

ACC-119: allow docker image deployment #34

wants to merge 3 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.

Invalidated as it needs more tests.

Their is also no apache configuration to expose the container. IMO at least a http / https and ldap protected / unprotected version should be need

env_var PRODUCT_BRANCH `expr "${PRODUCT_VERSION}" : '\([0-9]*\.[0-9]*\).*'`".x"
env_var PRODUCT_MAJOR_BRANCH `expr "${PRODUCT_VERSION}" : '\([0-9]*\).*'`".x"
configurable_env_var "INSTANCE_ID" ""
if [[ -v PRODUCT_VERSION ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

The -v option is not available on macos. Is it not possible to use a classical test with -z or -n ?

Why not using a test on PRODUCT_NAME == docker ?

fi
# docker image should have INSTANCE_KEY
if [ -n "${DEPLOYMENT_DOCKER_IMAGE}" ]; then
env_var "INSTANCE_KEY" "docker-${DEPLOYMENT_DOCKER_IMAGE}-${DOCKER_IMAGE_VERSION}"
Copy link
Contributor

Choose a reason for hiding this comment

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

You should not lose the INSTANCE_ID parameter if we want to be able to deploy several time the same image


# validate additional parameters
case "${ACTION}" in
start | stop | restart | undeploy | deploy | download-dataset)
# Mandatory env vars. They need to be defined before launching the script
validate_env_var "PRODUCT_NAME"
validate_env_var "PRODUCT_VERSION"
configurable_env_var "PRODUCT_VERSION" ""
Copy link
Contributor

Choose a reason for hiding this comment

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

The validate should be moved on the previous test to ensure it's defined

@@ -1258,6 +1273,8 @@ do_deploy() {
esac
# Hack
do_configure_chat

fi
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation looks weird

fi
# docker image should have INSTANCE_KEY
if [ -n "${DEPLOYMENT_DOCKER_IMAGE}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

In a plf deployment this variable is not set, the test will failed will failed

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