JSON plan

[odise]

Let's take this as a first attempt for the JSON plan feature. The JSON plan will be created whenever params.output_statefile is set true.

What do you think?

[ljfranklin]

PR is looking pretty good! Two more requests:

• Update the README to include the new behavior of output_statefile
• Add test: https://github.com/ljfranklin/terraform-resource/blob/master/DEVELOPMENT.md

If you don't have access to an S3 account to run the tests, take a shot a writing a test that compiles and I can run it and give feedback.

Thanks for taking the time to work on this!

[odise]

• Add test: https://github.com/ljfranklin/terraform-resource/blob/master/DEVELOPMENT.md

I'm fine with implementing tests for that however I see in_backend_test.go:297 failing now. I'm not so sure how to interpret this failure but maybe you could help me out here a bit. The metadata file is present but has {} content.

Couldn't test the GCP part due to missing account.

[odise]

I see in_backend_test.go:297 failing now. I'm not so sure how to interpret this failure but maybe you could help me out here a bit. The metadata file is present but has {} content.

I guess this is caused due to the change in inWithBackend. There was an early return in case of plan_only = true. I corrected this, test should be fine now.

[ljfranklin]

@odise doesn't look like the in_backend_test.go changes got pushed

[odise]

@odise doesn't look like the in_backend_test.go changes got pushed

For me all tests of the "In Suite" are ok. Have checked 7ade564?

[ljfranklin]

@odise oh, I thought you meant you already added a new test. Could you try writing a new test that exercises this feature?

[odise]

@odise oh, I thought you meant you already added a new test. Could you try writing a new test that exercises this feature?

Please have a look at 2714d5a. Its a bit off the schema you currently have for the tests as doesn't fit into in/out.

[ljfranklin]

Test looks great! Couple more minor comments and we should be good to merge.

[ljfranklin]

Can you combine this and the inner if statement into a single statement, i.e. if req.Version.IsPlan() && req.Params.OutputJSONPlanfile? If you're not pulling a plan version there's no plan file to download.

[odise]

Well you can use the in resource to pull the JSON plan without setting plan_only: true. And it is possible to get the plan as part of plan only run and use it further on. I'm not sure if a combined statement makes that much of a sense. Maybe I don't get the point here.

[ljfranklin]

version.IsPlan is different than params.PlanOnly. If IsPlan() is true that means you're pulling a version that corresponds to a put which used PlanOnly. The only time a plan file should exist in the backend is for versions where version.IsPlan() is true.

[odise]

What about a config like that:

  - name: terraform-check
    plan:
      - get: terraform-s3-backend
        params:
          output_planfile: true

This should retrieve the plan from the backend and provide the JSON representation. It is also the use case the test covers. If I set the condition to req.Version.IsPlan() && req.Params.OutputJSONPlanfile this will not provide the plan.json.

An other example would be the get after put step. Here the params.plan_only is true. If I set get_params.output_planfile true here I expect the plan.json as well.

In the current implementation the get after put will return right after the JSON plan. In native get it will continue and provide the state file if params.output_statefile is set.

[odise]

I extended the test and put some comments.

[ljfranklin]

If I set the condition to req.Version.IsPlan() && req.Params.OutputJSONPlanfile this will not provide the plan.json

Not sure what you mean. To make sure we're on the same page I'm saying change your second if block to the following:

if req.Params.OutputJSONPlanfile && req.Version.IsPlan() {
  		if err := r.writeJSONPlanToFile(targetEnvName+"-plan", client); err != nil {
			return models.InResponse{}, err
		}
                resp := models.InResponse{
			Version: req.Version,
		}
		return resp, nil
}

This should pass all your tests. If req.Version.IsPlan() is false then there is no plan stored in the backend so writeJSONPlanToFile will always fail which is why I'm saying to move the IsPlan() check into the parent if. This also handles the case where a user sets output_planfile under resource.source.output_planfile so the behavior applies to all puts in the pipeline (maybe not advised but should work).

[odise]

Here is what happens when I do the change.

• Failure in Spec Teardown (AfterEach) [92.775 seconds]
JSON Plan [AfterEach] plan infrastructure and test it
/Users/jan/Documents/code/go/src/github.com/ljfranklin/terraform-resource/src/terraform-resource/in/in_plan_test.go:202

  Expected
      <string>: /var/folders/kw/q4_t1zpx2yzd38fbl4s5s99c0000gn/T/terraform-resource-in-test764721631/plan.json
  to exist

  /Users/jan/Documents/code/go/src/github.com/ljfranklin/terraform-resource/src/terraform-resource/in/in_plan_test.go:278

Seems that we have a bit tricky situation here. I hope that we can sort that out somehow. Whether my test doesn't make sense or the change you are suggesting is not leading to the result I have in mind ;). Any ideas how to proceed?

[ljfranklin]

Ah, the issue is with your test setup. Replace:

			Version: models.Version{
				EnvName: envName,
				Serial:  "0",
			},

With:

Version: planOutput.Version,

That's more representative of a real run and should make things pass.

[odise]

Ok it seems we got it.

[ljfranklin]

@odise looks great, thanks for the PR! This is going through CI now, I'll give an update when it's available in the docker images.