nicgrayson / terraform-provider-marathon

a Terraform (http://terraform.io) provider for interacting with Marathon (https://mesosphere.github.io/marathon/)
MIT License
59 stars 24 forks source link

Wait for deployment on create #6

Closed DustinChaloupka closed 9 years ago

DustinChaloupka commented 9 years ago

Interestingly enough, this broke the tests, so those still need fixed.

Another thing possibly worth discussing is how to handle deployments that take an absurdly long time, namely docker images that are really large. The longest one I can think of is our jenkins master image, but I think that is under the 10 minute timeout. Is that fine? Or should we provide some sort of option for a marathon_app where we don't wait for the deployment? This could also help speed up terraform when we don't actually want to wait for something if there are cases.

lamdor commented 9 years ago

This looks like a good start. The deployments endpoint gives a little bit better vision into the deployment and the current status of a deployment: https://mesosphere.github.io/marathon/docs/rest-api.html#get-/v2/deployments We might want consider switching to that as we can give feedback of what stage it failed out. Thoughts?

DustinChaloupka commented 9 years ago

I was originally using that endpoint, but it was a bit more complicated since that just includes all deployments of all apps and there would only ever be one deployment for an app that was being created. However, I did not think about giving feedback on what stage it failed out on so I can go that route if we want it.

knuckolls commented 9 years ago

These endpoints are a clever solution to the problem. I had initially thought that terraform would likely have to hook into the event bus to get access to the "deployment is finished" event. It may be that if that information is only supplied on the event bus, then that's what we'll have to do.

DustinChaloupka commented 9 years ago

This should be good now unless we wanted to add some sort of functionality to a marathong_app to not wait for it's deployments to be done.

DustinChaloupka commented 9 years ago

I'm also not sure what is up with the "ports" needing to be defined. When they weren't the plan between the test steps was erroring:

--- FAIL: TestAccMarathonApp_basic (44.26s)
    testing.go:121: Step 0 error: After applying this step, the plan was not empty:

        DIFF:

        UPDATE: marathon_app.app-create-example
          ports.#: "1" => "0"
          ports.0: "10000" => "0"

        STATE:

        marathon_app.app-create-example:
          ID = /app-create-example
          app_id = /app-create-example
          args.# = 0
          backoff_factor = 1.15
          backoff_seconds = 1
          cmd = env && python3 -m http.server $PORT0
          container.# = 1
          container.0.docker.# = 1
          container.0.docker.0.image = python:3
          container.0.docker.0.network =
          container.0.docker.0.port_mappings.# = 0
          container.0.docker.0.privileged = true
          container.0.type = DOCKER
          container.0.volumes.# = 0
          cpus = 0.01
          dependencies.# = 0
          instances = 1
          mem = 100
          ports.# = 1
          ports.0 = 10000
          require_ports = false
          uris.# = 0
          version = 2015-04-15T15:32:49.707Z
lamdor commented 9 years ago

I'll check out that failing test here in a bit. Not sure why it's failing.

lamdor commented 9 years ago

requires https://github.com/Banno/go-marathon/pull/8

lamdor commented 9 years ago

It passed locally for me.

DustinChaloupka commented 9 years ago

@rubbish Yeah, I pushed up something that "fixed" the test: https://github.com/Banno/terraform-provider-marathon/commit/6801ca388cc4aa78e385cf80e25ddfc6481e909d

I just figured out what is going on. In the tests we are doing another read and it is setting the port to 10000 on the app.

Edit: Or not... but the testing is doing the plan differently it seems. Just doing an apply and then plan with the same configuration does not show the ports being changed in the plan.

lamdor commented 9 years ago

:+1:

DustinChaloupka commented 9 years ago

This and it's requirement should be good to go now if people want to take a look. /cc @knuckolls

knuckolls commented 9 years ago

reviewed the code. i decided that anything i'd mention was just nitpicking. this is good and i'm cool w/ merging it.