search

deis / workflow-e2e

End-to-end tests for Deis Workflow
MIT License
12 stars 18 forks source link

fix(healthcheck): use correct port the image exposes #320

Closed kmala closed 8 years ago

kmala commented 8 years ago

with this PR https://github.com/deis/controller/pull/1036 the healthchecks are properly checked and hence the e2e fails.

bacongobbler commented 8 years ago

I think instead of changing the existing tests, we should just add one test that confirms setting a healthcheck with a bad port will roll back. We still want to test good values in e2e.

kmala commented 8 years ago

@bacongobbler i will write it after i merge this as that test won't pass until https://github.com/deis/controller/pull/1036 is merged.

bacongobbler commented 8 years ago

Why not wait until after deis/controller#1036 is merged first so you save yourself from writing 2 separate PRs?

kmala commented 8 years ago

https://github.com/deis/controller/pull/1036 won't pass e2e until this is merged.

bacongobbler commented 8 years ago

Why is that? I'm probably missing some background context here that's not obvious to me (sorry)

kmala commented 8 years ago

this is not adding the e2e tests for the https://github.com/deis/controller/pull/1036 pr...this is fixing the existing e2e tests.

bacongobbler commented 8 years ago

The point I'm missing here is why this fixes the existing e2e tests. As far as I can tell e2e has been green for days.

kmala commented 8 years ago

e2e is green for now because the code doesn't have https://github.com/deis/controller/pull/1036 which fails a deploy if the healthcheck fails...current code doesn't do anything if the healthcheck fails and our deploys are failing with https://github.com/deis/controller/issues/989 in e2e but we are not checking it as the command exists 0 status.

bacongobbler commented 8 years ago

So the missing context that was never provided was "where did port 1500 and port 8080 come from"?

It turns out that port 1500 is the port number for deis/example-dockerfile-http, and port 8080 is for quay.io/deisci/e2e-private-registry-test. In that case, can you please remove this comment and add a new comment stating why port 1500 and 8080 are significant?

kmala commented 8 years ago

they are the actual ports of the images we are deploying. i can change that test case.