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

Auto-detection of terraform executable & downloading #478

Merged
merged 18 commits into from
Jun 4, 2021

Conversation

eerkunt
Copy link
Member

@eerkunt eerkunt commented May 9, 2021

This PR will introduce new argument for providing a terraform version where it will be downloaded on the first run (and then cached). This was a feature request especially for docker users where we always download the latest version in the image.

@eerkunt
Copy link
Member Author

eerkunt commented May 9, 2021

This PR fixes #365

@eerkunt
Copy link
Member Author

eerkunt commented May 9, 2021

#381

@eerkunt
Copy link
Member Author

eerkunt commented May 9, 2021

@eerkunt
Copy link
Member Author

eerkunt commented May 9, 2021

Also fixes #273

@eerkunt eerkunt changed the title New argument: -tv / --terraform-version Auto-detection of terraform executable & downloading May 10, 2021
@eerkunt
Copy link
Member Author

eerkunt commented May 10, 2021

Changed the way how this works. There won't be any more parameter as -tv / --terraform-version instead, terraform-compliance will attempt a detection of which version of terraform is required. Then it will download & use that executable on converting the plan file into JSON.

Copy link
Member

@Kudbettin Kudbettin left a comment

Choose a reason for hiding this comment

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

I am having a couple of concerns with this PR:

No Terraform executables

If there are no terraform executables at all, the feature won't attempt downloading it. Is this intended?

Terraform executable exists

If there is a terraform executable but with wrong version:
Running terraform-compliance resulted with the following output

Screen Shot 2021-05-10 at 10 16 05 AM

  • Firstly, error message is wrong, need to use the path of the newly installed terraform executable.

  • In this case, shouldn’t terraform-compliance automatically try to fix the issue by creating the json for me, instead of prompting me to convert the plan manually and run terraform-compliance again?

Running the recommended command with correct terraform executable didn’t immediately convert my plan.out file to json:

Screen Shot 2021-05-10 at 10 17 58 AM

The current setup I am testing with only has a plan.out file (and no terraform files or .terraform directory). Was I suppose to terraform init and setup the correct providers by hand? Or was terraform-compliance supposed to do those automatically as well.

Might need more documentation on how to use this functionality and/or some changes so that there are less steps to be taken by the user to convert the plan to json and run the scenario.

Testing

It would be preferable if we had a testing harness that spins up a few containers and tests this feature.

docs/pages/usage/index.md Outdated Show resolved Hide resolved
docs/pages/usage/index.md Outdated Show resolved Hide resolved
terraform_compliance/main.py Outdated Show resolved Hide resolved
Comment on lines +94 to +104
if 'Could not satisfy plugin requirements' in terraform.stderr:
print('Hint: You can avoid this problem by converting your plan file to a JSON file via running;\n '
'\n # terraform show -json {} > {}.json'
'\n\n OR'
'\n # terraform init'
'\n\n in {} directory and then pass (with -p) {}.json to terraform-compliance'.format(terraform_plan_file,
terraform_plan_file,
path,
terraform_plan_file))

sys.exit(1)
Copy link
Member

@Kudbettin Kudbettin May 10, 2021

Choose a reason for hiding this comment

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

Should print out the path of the correct executable if it was just downloaded. More about this below above

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm this can only happen where a -t is used and a wrong version is enforced to use while creating the plan, right ? 🤔

In that case, we don't download anything,

@eerkunt
Copy link
Member Author

eerkunt commented May 10, 2021

The reason why you get the init errors, is because all you have is just a plan.out, not the tf files. Unfortunately, terraform requires a init before running a show command 🤷‍♂️ This is something that we can't control, hence IMHO we need to emphasise the use of plan.out.json instead of plan.out

Converting plan.out is just brings too many problems.

@Kudbettin
Copy link
Member

Kudbettin commented May 12, 2021

Alright. I have misunderstood the use case then. I thought this change would handle everything regarding converting the different version plan.out to a JSON format. The user should still have .terraform directory close to the plan for auto-detection & downloading to work.

I have tested this in two setups.

Case 1: running a 0.13.0 plan with 0.15.3 (of Terraform)

  • master failed to automatically create plan.out.json
  • This branch did download 0.15.3 and converted the file correctly

Case 2: running a 0.15.3 plan with 0.13.0 (of Terraform)

  • master failed to automatically create plan.out.json
  • This branch failed to automatically create plan.out.json
  • The output is as follows (for both branches)

Screen Shot 2021-05-11 at 9 55 51 PM

Should this feature be able to handle Case 2 as well?

@eerkunt
Copy link
Member Author

eerkunt commented May 12, 2021

You are right it should. Released 1.3.15 without this functionality, will test & improve this PR.

@eerkunt
Copy link
Member Author

eerkunt commented May 12, 2021

This PR should also have a signature verification after the download as well.

@eerkunt
Copy link
Member Author

eerkunt commented Jun 4, 2021

@Kudbettin the use case you mentioned is fixed on 04978f0

Copy link
Member

@Kudbettin Kudbettin left a comment

Choose a reason for hiding this comment

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

All my test cases are passing now. Looks good to me!

@eerkunt eerkunt merged commit 2adb1dc into master Jun 4, 2021
@eerkunt eerkunt deleted the terraform-version branch June 4, 2021 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants