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

Terraform version config detection #789

Merged

Conversation

YesYouKenSpace
Copy link
Contributor

@YesYouKenSpace YesYouKenSpace commented Sep 25, 2019

TLDR

Assumptions made

  1. Only when the user specify an exact version in the terraform configuration then do we use it. The specification requirements are in https://www.terraform.io/docs/configuration/terraform.html#specifying-a-required-terraform-version

    = (or no operator): exact version equality

    Anything else doesn't really make sense, we can fall back on server config as explained below(##Assumptions.2).

  2. Version specified in atlantis.yaml takes precedence:
    atlantis.yaml repo config > terraform.required_version > defaultTfVersion in atlantis server config
    This way requires the least change to existing code.

Use of github.com/hashicorp/go-version

Offload interpretation of version number to the library.

Closes #71

@YesYouKenSpace YesYouKenSpace force-pushed the terraform-version-config-detection branch from f49d253 to c5384c7 Compare September 25, 2019 18:03
@YesYouKenSpace
Copy link
Contributor Author

I don't understand. Why is the CI failing? Seems like it had trouble installing the dependency.

@YesYouKenSpace YesYouKenSpace force-pushed the terraform-version-config-detection branch from c5384c7 to 4e696bb Compare September 28, 2019 16:53
@YesYouKenSpace
Copy link
Contributor Author

@lkysow This is working on when I do make test-coverage on my local. Is this because it is using the testing image in the ci pipeline?

@lkysow
Copy link
Member

lkysow commented Oct 1, 2019

You probably need to run go mod vendor since we run everything in vendor mode.

@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@75da89e). Click here to learn what that means.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #789   +/-   ##
=========================================
  Coverage          ?   72.78%           
=========================================
  Files             ?       63           
  Lines             ?     4836           
  Branches          ?        0           
=========================================
  Hits              ?     3520           
  Misses            ?     1055           
  Partials          ?      261
Impacted Files Coverage Δ
server/events/project_command_builder.go 82.2% <78.57%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75da89e...1c624bd. Read the comment docs.

@YesYouKenSpace
Copy link
Contributor Author

@lkysow Ahhhh awesome thanks! Fixed!!! Love to hear your feedback/review for this PR! :)

@YesYouKenSpace
Copy link
Contributor Author

YesYouKenSpace commented Oct 2, 2019

Actually I was thinking, with this PR we can output useful information for the users such as VAULT_ADDR defined in the vault providers, etc. Let me know what you think about this. I can do them in another PR. Wanna keep this one small-reviewable.

Some use cases:

Combined with https://github.com/runatlantis/atlantis/blob/master/CHANGELOG.md#v090 . There is potential to select the right way to generate relevant environment variables for the providers use. We only have to provide the information, users can write the logic themselves.

@zytek
Copy link

zytek commented Oct 3, 2019

We are in the process of migrating from TF 0.11 to TF 0.12 across our mono-repo with multiple terraform "modules" (subdirs). Currently we use atlantis automatic mode for project detection, so we have no place to specify per-module terraform version. We bump required_version in each module during update process. This PR seems to ease our migration effort a lot, so thanks! Gonna give it a try this or the next week.

@YesYouKenSpace
Copy link
Contributor Author

Thanks @zytek ! Let me know if it breaks anything.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Looking good! I have a couple comments.
Also can you split up the vendor updates and the code changes into two commits. I will want to do the vendoring myself just to be safe.

server/events/project_command_builder.go Outdated Show resolved Hide resolved
server/events/project_command_builder.go Outdated Show resolved Hide resolved
testing/temp_files.go Show resolved Hide resolved
server/events/project_command_builder_test.go Show resolved Hide resolved
@lkysow
Copy link
Member

lkysow commented Oct 4, 2019

Actually I was thinking, with this PR we can output useful information for the users such as VAULT_ADDR defined in the vault providers, etc. Let me know what you think about this. I can do them in another PR. Wanna keep this one small-reviewable.

Some use cases:

Combined with https://github.com/runatlantis/atlantis/blob/master/CHANGELOG.md#v090 . There is potential to select the right way to generate relevant environment variables for the providers use. We only have to provide the information, users can write the logic themselves.

This sounds cool. In a separate ticket I'd be interested in some concrete use-cases.

@YesYouKenSpace YesYouKenSpace force-pushed the terraform-version-config-detection branch 4 times, most recently from 9d9f5d8 to 2d2e51f Compare October 6, 2019 07:30
@YesYouKenSpace
Copy link
Contributor Author

Looking good! I have a couple comments.
Also can you split up the vendor updates and the code changes into two commits. I will want to do the vendoring myself just to be safe.

Splitted the pr into two commits

@byakku
Copy link

byakku commented Oct 7, 2019

@kennethtxytqw

Just tested newest version (8c676c2ff49e2eeaff038a3f05b68e442f85f0f3), it does not detect nor use good version.

What I expected:
Atlantis will dynamically choose correct terraform version based on atlantis.yaml or required_version in terraform.tf.

What is actually happening:
Atlantis picks default version and keeps using it despite having different value in terraform.tf.

When there is no default version, I've ran atlantis plan and it executed correctly (plan failed because module was using 0.11, not 0.12) and atlantis selected 0.12.9 as default version.

When I ran atlantis plan in repo with modules using 0.11 and 0.12 only those with 0.12 got planned properly, atlantis didn't detect/select other version correcty.

terraform-config-inspect returns correct version (specified in terraform.tf)

How to reproduce:
(Ofc build custom image based on this PR)

  1. Create repository with at least 2 subdirs. Put any working .tf there
  2. In dir1 put required_version = "0.12.9" in terraform.tf
  3. In dir2 put required_version = "0.11.14" in terraform.tf
  4. Trigger atlantis.

In logs I could see cannot determine required core from terraform configuration. in case with multi-directory PR containing 2 different terraform versions.

@YesYouKenSpace YesYouKenSpace force-pushed the terraform-version-config-detection branch from 8c676c2 to 51ddfbb Compare October 7, 2019 15:57
@YesYouKenSpace
Copy link
Contributor Author

Odd. I just added a test for multiple directory repo. It worked.

@zytek
Copy link

zytek commented Oct 8, 2019

@kennethtxytqw any idea how can we assist in testing this? Our setup with @byakku is rather simple. One repo with a few dirs (ecr/ s3/ iam/ etc) and in each dir there is terraform.tf file with requires_version = "x.y.z" - a mix o 0.11 and 0.12. Atlantis default is set up to 0.11.14. And autodetection does not work.

edit: we have no atlantys.yaml file and rely only on atlantis autodiscovery/autodetection of projects

Example output from terraform-confit-inspect running in a dir:

➜  s3 git:(master) ✗ ~/go/bin/terraform-config-inspect

# Module `.`

Core Version Constraints:
* `0.12.9`
* `>= 0.12`

Provider Requirements:
* **aws:** (any version)
[trim]

@YesYouKenSpace
Copy link
Contributor Author

@zytek Why are there two ? Did you specify required_version in two places for the project?

Core Version Constraints:
* `0.12.9`
* `>= 0.12`

@zytek
Copy link

zytek commented Oct 14, 2019

@kennethtxytqw we had version specified in terraform.tf file and a second one in versions.tf created by probably by terraform 0.12upgrade command that we had missed out.

Atlantis could provide more helpful error message in such case :)

Testing this PR agains proper project with only one version constrain right now.

re := regexp.MustCompile(`^=?\s*([^\s]+)\s*$`)
matched := re.FindStringSubmatch(module.RequiredCore[0])
if len(matched) == 0 {
ctx.Log.Warn("did not specify exact version in terraform configuration.")
Copy link

Choose a reason for hiding this comment

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

I would unify log levels here or switch those message priorities as the fact that a version was detected and is being use is IMHO more important than the fact that we fallback to defaults because no version was detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm you make sense. I will update my PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some more logs and unified as you suggested.

@zytek
Copy link

zytek commented Oct 14, 2019

I can confirm now that this PR works as expected. One note added regarding log levels.

Just remember to not shoot yourself in the foot by leaving autogenerated versions.tf file and by deploying wrong docker image of atlantis ;-)

@YesYouKenSpace
Copy link
Contributor Author

YesYouKenSpace commented Oct 15, 2019

I can confirm now that this PR works as expected. One note added regarding log levels.

Just remember to not shoot yourself in the foot by leaving autogenerated versions.tf file and by deploying wrong docker image of atlantis ;-)

HAHA got it! Thanks for testing it out. I tested it out too with this image https://hub.docker.com/r/yesyouken/atlantis feel free to use this one for testing

@YesYouKenSpace YesYouKenSpace force-pushed the terraform-version-config-detection branch from 51ddfbb to ceffad7 Compare October 15, 2019 15:09
@YesYouKenSpace YesYouKenSpace force-pushed the terraform-version-config-detection branch from 35a83d5 to 1c624bd Compare October 15, 2019 15:19
@YesYouKenSpace YesYouKenSpace requested a review from lkysow October 20, 2019 06:25
@YesYouKenSpace
Copy link
Contributor Author

I have tested this at my workplace for more than a week already. We have deprecated the use of terraform_version in atlantis.yaml and depend on the terraform configuration completely. Thanks for the help guys!

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Nice work, awesome PR

@lkysow lkysow merged commit a02a663 into runatlantis:master Oct 28, 2019
@MPV
Copy link

MPV commented Feb 4, 2020

Anyone up for adding a mention of this in the documentation? 😅 https://www.runatlantis.io/docs/terraform-versions.html

Took a while for me to find this PR and see that this was actually really supported. ❤️

@lkysow
Copy link
Member

lkysow commented Feb 4, 2020

YesYouKenSpace pushed a commit to YesYouKenSpace/atlantis that referenced this pull request Feb 8, 2020
@YesYouKenSpace
Copy link
Contributor Author

My bad should have done so in the same PR.

@llamahunter
Copy link
Contributor

Why do general version constraints on the terraform.required_version 'not make sense'?

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.

Detect Terraform version from terraform.required_version block
6 participants