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

Updates endpoint for web container readiness check #82

Merged
merged 4 commits into from
Mar 17, 2017

Conversation

tannerjfco
Copy link
Contributor

@tannerjfco tannerjfco commented Mar 17, 2017

The Problem:

#66 original issue desc. We need to reliably determine if the web container is ready to serve.

The Fix:

This updates to check the new /healthcheck endpoint added to the nginx containers, rather than the root of the user's web application. This will allow us to determine container readiness regardless of the web app's state.

This also restores the Wait() in the start cmd, and updates restart to remove Config(), as that should have been removed.

The Test:

  • Pick your favorite test site and run a a ddev start. Previously, the start cmd did not check readiness. You should now see that it is doing so.
  • Run a ddev restart. Previously, this would have written a settings.php or wp-config.php, and would have failed status check as it was checking an uninitialized site. There should be no config file written, and the status check should succeed.

Automation Overview:

The local plugin tests have been adjusted to include Wait() in TestLocalStart() and TestLocalRemove().

Related Issue Link(s):

#66
drud/docker.nginx-php-fpm#33
drud/docker.nginx-php-fpm-local#7

Release/Deployment notes:

Does this affect anything else, or are there ramifications for other code? Does anything have to be done on deployment?

These PRs are part of this change:
drud/docker.nginx-php-fpm#33
drud/docker.nginx-php-fpm-local#7

Provisional tags for v0.3.1 have been pushed to dockerhub. Upon merge in both repos, a new v0.3.1 release should be tagged.

@tannerjfco tannerjfco added this to the v0.1 milestone Mar 17, 2017
@tannerjfco tannerjfco self-assigned this Mar 17, 2017
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I tested this manually and am good with it, so approving. Please consider if you can or should add tests for this feature.

@@ -49,8 +50,15 @@ var StartCmd = &cobra.Command{
Failed("Failed to start %s: %s", app.GetName(), err)
}

Success("Successfully added %s", activeApp)
Success("Your application can be reached at: %s", app.URL())
fmt.Println("Waiting for site readiness. This may take a couple minutes...")
Copy link
Member

Choose a reason for hiding this comment

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

No biggie, but "couple of minutes" would be more idiomatic. The actual total time on my 2013 Macbook Air was 27 seconds total for ddev start from beginning to end, but the containers had already been cached. However, all that is before this statement. Might even be OK to say "a minute or more" here.

@tannerjfco tannerjfco merged commit a55e0bf into ddev:master Mar 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants