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.

Sign up for GitHub

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

Jump to bottom

[cli] Fix #3655 : Check docker-compose file validity, result of docker-compose up and containers #3731

Merged
merged 2 commits into from
Jan 17, 2017
Merged

[cli] Fix #3655 : Check docker-compose file validity, result of docker-compose up and containers #3731

merged 2 commits into from
Jan 17, 2017

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Jan 13, 2017

Fix for #3655

  • check validity of docker-compose file by calling config method
  • check result of "docker-compose up" command
  • check each container of docker-compose services if they’re running or not

Change-Id: Ie7ef9cc301c2605ec40db70f00d06f1fca694e4c
Signed-off-by: Florent BENOIT fbenoit@codenvy.com

@benoitf
Fix #3655 : Check docker-compose file validity, result of docker-comp…
af600b8 
…ose up command and also check each container of docker-compose services if they’re running or not

Change-Id: Ie7ef9cc301c2605ec40db70f00d06f1fca694e4c
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>
@benoitf benoitf added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Jan 13, 2017
@benoitf benoitf self-assigned this Jan 13, 2017
@benoitf benoitf mentioned this pull request Jan 13, 2017
Closed
TylerJewell
TylerJewell suggested changes Jan 13, 2017
View reviewed changes
dockerfiles/base/scripts/base/commands/cmd_start.sh
@@ -45,12 +45,16 @@ cmd_start() {
info "start" "Starting containers..."
COMPOSE_UP_COMMAND="docker_compose --file=\"${REFERENCE_CONTAINER_COMPOSE_FILE}\" -p=\"${CHE_MINI_PRODUCT_NAME}\" up -d"

## validate the compose file (quiet mode)
docker_compose --file=${REFERENCE_CONTAINER_COMPOSE_FILE} config -q || (error "Invalid docker compose file content at ${REFERENCE_CONTAINER_COMPOSE_FILE}" && return 2)
Copy link

Choose a reason for hiding this comment

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

What are the circumstances where the end user has provided a valid che.env, but this configuration check would fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe with only che.env it's not possible but a dev could break the docker-compose file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added that check only if "local_repo" is used.

dockerfiles/base/scripts/base/commands/cmd_start.sh
if ! debug_server; then
COMPOSE_UP_COMMAND+=" >> \"${LOGS}\" 2>&1"
fi

log ${COMPOSE_UP_COMMAND}
eval ${COMPOSE_UP_COMMAND}
eval ${COMPOSE_UP_COMMAND} || (error "Error while running compose up command" && tail -30 ${LOGS} && error "The command compose UP has failed. Latest 30lines of log files ${CHE_HOST_CONFIG}/cli.log have been printed." && return 2)
Copy link

Choose a reason for hiding this comment

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

I think this would be a bit more concise:

(error "Error during 'compose up' - printing 30 line tail of ${CHE_HOST_CONFIG}/cli.log:" && tail -30 ${LOGS} && return 2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

dockerfiles/base/scripts/base/commands/cmd_start.sh
return 2
fi
done <<< "${LIST_OF_COMPOSE_CONTAINERS}"
}
Copy link

Choose a reason for hiding this comment

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

newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

dockerfiles/base/scripts/base/commands/cmd_start.sh
@@ -136,8 +140,10 @@ check_if_booted() {
info "start" "Server logs at \"docker logs -f ${CHE_SERVER_CONTAINER_NAME}\""
fi

check_containers_are_running
Copy link

Choose a reason for hiding this comment

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

Why do you need to check this before the server is booted command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to check that before and after the containers are still running

@codenvy-ci
Copy link

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1632/

@TylerJewell TylerJewell self-requested a review January 13, 2017 20:25
riuvshin
riuvshin approved these changes Jan 15, 2017
View reviewed changes
Copy link
Contributor

@riuvshin riuvshin left a comment

Choose a reason for hiding this comment

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

+1 but please add requested changes from @TylerJewell

@benoitf
fixup! Fix #3655 : Check docker-compose file validity, result of dock…
13e7f27 
…er-compose up command and also check each container of docker-compose services if they’re running or not

Change-Id: I91f0298aaaa3c547925989f964a8d2c38f6393d3
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>
@codenvy-ci
Copy link

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1641/

@benoitf
Copy link
Contributor Author

benoitf commented Jan 17, 2017

@TylerJewell are the changes ok for you now ?

@TylerJewell
Copy link

Yes - ok by me.

@benoitf benoitf merged commit 59f5da2 into master Jan 17, 2017
@benoitf benoitf deleted the CHE-3655 branch January 17, 2017 13:47
@benoitf benoitf added this to the 5.1.0 milestone Jan 17, 2017
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jan 17, 2017
@JamesDrummond JamesDrummond mentioned this pull request Jan 31, 2017
Closed
9 tasks
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
@benoitf
[cli] Fix eclipse-che#3655 : Check docker-compose file validity, resu…
41d45e7 
…lt of docker-compose up and containers (eclipse-che#3731)

* Fix eclipse-che#3655 : Check docker-compose file validity, result of docker-compose up command and also check each container of docker-compose services if they’re running or not

Change-Id: Ie7ef9cc301c2605ec40db70f00d06f1fca694e4c
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

* fixup! Fix eclipse-che#3655 : Check docker-compose file validity, result of docker-compose up command and also check each container of docker-compose services if they’re running or not

Change-Id: I91f0298aaaa3c547925989f964a8d2c38f6393d3
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riuvshin riuvshin riuvshin approved these changes

@TylerJewell TylerJewell TylerJewell approved these changes

Assignees

@benoitf benoitf

Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Milestone
5.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@benoitf @codenvy-ci @TylerJewell @riuvshin