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

[#115320097] Terraform 0.6.13 #38

Merged
merged 3 commits into from
Mar 22, 2016
Merged

[#115320097] Terraform 0.6.13 #38

merged 3 commits into from
Mar 22, 2016

Conversation

saliceti
Copy link
Contributor

What

This was created to investigate issue AWS: Can't update certificate associated with ELB in story ELB for access to logs.

This has not fixed that particular issue, but the whole pipeline was tested on that terraform version and it worked fine.

There is a also a minor improvement as the Alpine version is now pinned to 3.3.

How to review

Use image: docker:///governmentpaas/terraform#115320097-terraform-0.6.13 in the pipeline and deploy.

Who can review

Anyone but @henrytk or myself

@@ -23,6 +23,14 @@
expect(file("/etc/alpine-release")).to be_file
end

it "installs the right version of Alpine" do
expect(os_version).to include("Alpine Linux 3.3")
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I see the value in testing this. Do we care what version of alpine this is? All we care about is that the container has the necessary tools, and that they work.

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 feel more comfortable pinning the versions of dependencies so upgrading terraform version is a minimal change, instead or risking unexpected behaviour due to dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm 👍 for pinning the version, but 👎 for testing the specific version.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we stop checking the Alpine version should we also remove the Terraform version check on line 55?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what's the value of pinning the version and running tests that do not check it?
If we want to be sure that terraform is in specific version as it contains fixes that we want then we should check if this specific version was installed. And in my opinion the same applies to any other software that we use including OS.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we stop checking the Alpine version should we also remove the Terraform version check on line 55?

We shouldn't remove it, because we care about the specific version of Terraform and have written code in the Dockerfile to use the version in the environment variable. We'd want to know if (hypothetically) someone changed the URL to "latest" (if that exists) or the Terrraform website provided the wrong version.

Colin and I talked a bit about testing the Alpine version. I think it makes more sense to test that it's Alpine (because we want a minimal container and ability to use apk commands) rather than the specific version (which we want for pinning but not because of that specific version).

Though there's a separate can of worms about whether we should be pinning the versions of packages within an Alpine series.

@alext
Copy link
Contributor

alext commented Mar 18, 2016

It would be good to also update the version of terraform installed in the paas-cf Travis.yml file so that the unit tests use the same version.

@mtekel
Copy link
Contributor

mtekel commented Mar 18, 2016

This version has a lot of good fixes, e.g.:

  • provider/aws: aws_instance now allows changes to security groups without force new resource (#5193)
  • core: Includes upstream HCL fix to properly detect unbalanced braces and throw an error (#5400)

@dcarley
Copy link
Contributor

dcarley commented Mar 21, 2016

The build is failing for some reason that I don't understand:

Step 2 : RUN apk add --update $PACKAGES && rm -rf /var/cache/apk/*
 ---> Running in 8550d96c7efd
fetch http://dl-cdn.alpinelinux.org/alpine/v3.3/main/x86_64/APKINDEX.tar.gz
fetch http://dl-cdn.alpinelinux.org/alpine/v3.3/community/x86_64/APKINDEX.tar.gz
ERROR: unsatisfiable constraints:
  openssh-client-7.2_p2-r0:
    breaks: world[openssh-client=7.1_p2-r0]
The command '/bin/sh -c apk add --update $PACKAGES && rm -rf /var/cache/apk/*' returned a non-zero code: 1

@saliceti could you look at this and address the comments. I'm 👍 for upgrading

@mtekel
Copy link
Contributor

mtekel commented Mar 21, 2016

There is some indication that 0.6.13 might fix the "flapping" NAT gateway route table updates, see latest comments in hashicorp/terraform#4097 (comment) and our story Fix issue with Terraform always updating routing tables for better description of the issue.

@saliceti
Copy link
Contributor Author

The build issue was due to the git-ssh container. It depends on openssh-client package, but the version it relies upon was removed from the index. Bumping the version fixes the build.

It questions wether we should pin versions at all as it looks like Alpine doesn't keep previous versions.

This was tested against the whole CF pipeline to test if a particular
issue was fixed. This issue is still present in this version, but since it
doesn't break the pipeline we may as well update terraform.
The openssh-client 7.1p2 version was removed from the index and this
was breaking the whole build.
We choose now to not pin the version of individual packages as:

* the versions change too quickly
* there is not much value in pinning down to individual packages
* there is additional burden for security fixes

The version of Alpine is pinned, but we don't test against it to avoid
maintenance burden.
We pin the version to guarantee some stability but we don't test
for that version in specs to avoid to maintain the version in
2 places.
@saliceti
Copy link
Contributor Author

Comments addressed

@dcarley dcarley self-assigned this Mar 22, 2016
dcarley added a commit that referenced this pull request Mar 22, 2016
@dcarley dcarley merged commit a7f8329 into master Mar 22, 2016
@dcarley dcarley deleted the 115320097-terraform-0.6.13 branch March 22, 2016 13:44
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.

6 participants