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

Build Network Fixtures and Run Tests with Kitchen-Terraform #33

Merged
merged 39 commits into from
Dec 19, 2018

Conversation

Jberlinsky
Copy link
Contributor

@Jberlinsky Jberlinsky commented Nov 20, 2018

Continuation of #20, incorporating #20 (comment).

Addresses #28 and #25 along the way.

Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

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

We should try to limit the use of attributes to values which are necessary to obtain subjects. Relying on an attribute to provide the expected value of an expectations may indicate that the expectation is simply validating that the Terraform resources applied correctly rather than verifying broader intent of the configuration.

We should try to limit expectations to 1 per example in order to provide clarity if expectations fail.

Using the includes and including matchers could help to simplify the examples which rely on filtered values.

let(:taints) { nodes.first.spec.taints.map(&:to_h) }
# ...
expect(taints).to include(
  including(
    effect: "PreferNoSchedule",
    key: "all-pools-example",
    value: "true"
  ),
  # ...
)

Makefile Outdated Show resolved Hide resolved
test/integration/node_pool/controls/gcloud.rb Outdated Show resolved Hide resolved
test/integration/node_pool/controls/gcloud.rb Outdated Show resolved Hide resolved
test/integration/node_pool/controls/gcloud.rb Outdated Show resolved Hide resolved
test/integration/simple_regional/controls/gcloud.rb Outdated Show resolved Hide resolved
test/integration/simple_zonal/controls/gcloud.rb Outdated Show resolved Hide resolved
test/integration/node_pool/controls/gcloud.rb Outdated Show resolved Hide resolved
test/integration/node_pool/controls/gcloud.rb Outdated Show resolved Hide resolved
test/integration/simple_zonal/controls/gcloud.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

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

I have verified that the tests pass. stub_domains requires 2 converges to correctly configure module.gke.google_container_cluster.primary.

@morgante
Copy link
Contributor

morgante commented Dec 6, 2018

@Jberlinsky Here are follow-up items:

  • Converge testing documentation and approach with project factory. We should be pushing people to run tests in Docker to minimize differences.
  • I think the approach of manually managing fixtures by copying data into arbitrary example directories is very brittle and we need to back out of it ASAP. Principle: I should be able to nuke my local test directory and all testing artifacts will disappear.
  • I'm still ambivalent about the approach of embedding network setup in the tests. In particular, I'm concerned about how we test shared VPC usage - do we have a plan for that?
  • kitchen converge fails:
       Error: Error applying plan:
       
       1 error(s) occurred:
       
       * module.gke.google_container_cluster.primary: Resource 'data.google_compute_network.gke_network' not found for variable 'data.google_compute_network.gke_network.self_link'
  • kitchen destroy fails if converge failed:
       Error: Error applying plan:
       
       1 error(s) occurred:
       
       * module.gke.local.cluster_master_auth_list_layer2: local.cluster_master_auth_list_layer2: At column 40, line 1: list "local.cluster_master_auth_list_layer1" does not have any elements so cannot determine type. in:
       
       ${local.cluster_master_auth_list_layer1[0]}

@Jberlinsky
Copy link
Contributor Author

@morgante I haven't seen the kitchen converge error you mentioned before. Could you expand on how your project was set up, and possibly provide a full log of the test run?

@Jberlinsky
Copy link
Contributor Author

Jberlinsky commented Dec 13, 2018

@morgante I've addressed the action items you listed above, based on our conversation on Thursday:

Could you take another look at this when you get a moment?

@Jberlinsky
Copy link
Contributor Author

Worth pointing out that 7e0c063 addresses #23 in passing.

Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

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

Just some small changes and clarifications requested.

test/fixtures/deploy_service/example.tf Outdated Show resolved Hide resolved
test/fixtures/deploy_service/outputs.tf Show resolved Hide resolved
test/make.sh Show resolved Hide resolved
.ruby-version Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Jberlinsky
Copy link
Contributor Author

@aaron-lane I've addressed your comments -- would appreciate a re-review!

aaron-lane
aaron-lane previously approved these changes Dec 14, 2018
@Jberlinsky Jberlinsky force-pushed the feature/automatic-fixture-network-creation branch from 2bbf53a to 12350cc Compare December 14, 2018 17:20
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Overall this is in much better shape than before. Still a few small warts.

.kitchen.yml Outdated Show resolved Hide resolved
cluster_zonal.tf Outdated Show resolved Hide resolved
examples/node_pool/main.tf Outdated Show resolved Hide resolved
examples/shared_vpc/README.md Outdated Show resolved Hide resolved
test/fixtures/all_examples/install.sh Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
cluster_zonal.tf Show resolved Hide resolved
@@ -39,7 +39,9 @@ mkdir "${TMPDIR}"

export KUBECONFIG="${TMPDIR}/config"

echo "${CA_CERTIFICATE}" | base64 --decode > "${TMPDIR}/ca_certificate"
# shellcheck disable=SC1117
base64 --help | grep "\--decode" && B64_ARG="--decode" || B64_ARG="-d"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems very complex? Why do we need to do all this greping?

Anyways. This is not a testing change = separate PR please.

Copy link
Contributor

Choose a reason for hiding this comment

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

This identifies which flag is used to "decode"; the interface of base64 differs across platforms.

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'd argue that this is indeed a testing change -- in moving from running these tests on the local host (the current state of master) to docker, it was necessary to change the interface of base64 as Aaron mentioned above depending on where the test is being run.

scripts/wait-for-cluster.sh Show resolved Hide resolved
test/fixtures/shared/variables.tf Outdated Show resolved Hide resolved
@morgante
Copy link
Contributor

Running kitchen converge twice causes a permadiff for networking:

         network:    "projects/clf-cft-testing-dev/global/networks/cft-gke-test-cwky" => "https://www.googleapis.com/compute/v1/projects/clf-cft-testing-dev/global/networks/cft-gke-test-cwky"
         subnetwork: "projects/clf-cft-testing-dev/regions/us-east4/subnetworks/cft-gke-test-cwky" => "https://www.googleapis.com/compute/v1/projects/clf-cft-testing-dev/regions/us-east4/subnetworks/cft-gke-test-cwky"

This is possibly an upstream issue.

@Jberlinsky Jberlinsky force-pushed the feature/automatic-fixture-network-creation branch from f6ca3a7 to 41a068d Compare December 19, 2018 02:48
@Jberlinsky
Copy link
Contributor Author

@morgante At a quick glance, the permadiff does indeed look to be upstream. I'm digging into it now.

@Jberlinsky
Copy link
Contributor Author

Jberlinsky commented Dec 19, 2018

It does indeed look like the permadiff is upstream -- all the way up to terraform core. See:

The good news is that as of 15 days ago, it's claimed to be fixed in master.

aaron-lane and others added 25 commits December 19, 2018 13:13
This avoids a missing file warning during subsequent executions of the
script.
This fixes the following build issue:
```
ERROR: unsatisfiable constraints:
  go-1.10.5-r0:
    breaks: world[go=1.10.1-r0]
```
This fixes the following build issue:
```
ERROR: unsatisfiable constraints:
  curl-7.61.1-r1:
    breaks: world[curl=7.61.1-r0]
  git-2.18.1-r0:
    breaks: world[git=2.18.0-r0]
```
This fixes the following build issue:
```
ERROR: unsatisfiable constraints:
  ca-certificates-20171114-r3:
    breaks: world[ca-certificates=20161130-r0]
    satisfies: curl-7.61.1-r1[ca-certificates]
               libcurl-7.61.1-r1[ca-certificates]
               .ruby-rundeps-0[ca-certificates]
  bash-4.4.19-r1:
    breaks: world[bash=4.3.42-r5]
  curl-7.61.1-r1:
    breaks: world[curl=7.60.0-r1]
  musl-dev-1.1.19-r10:
    breaks: world[musl-dev=1.1.14-r16]
    satisfies: libc-dev-0.7.1-r0[musl-dev]
  g++-6.4.0-r9:
    breaks: world[g++=5.3.0-r0]
  git-2.18.1-r0:
    breaks: world[git=2.8.6-r0]
  jq-1.6_rc1-r1:
    breaks: world[jq=1.5-r2]
  make-4.2.1-r2:
    breaks: world[make=4.1-r1]
  python2-2.7.15-r1:
    breaks: world[python=2.7.14-r0]
    satisfies:
               python2-dev-2.7.15-r1[python2=2.7.15-r1]
               py-setuptools-39.1.0-r0[python2]
               py2-pip-10.0.1-r0[python2]
  py2-pip-10.0.1-r0:
    breaks: world[py-pip=8.1.2-r0]
  python2-dev-2.7.15-r1:
    breaks: world[python-dev=2.7.14-r0]
```
@Jberlinsky Jberlinsky force-pushed the feature/automatic-fixture-network-creation branch from b16c1d3 to 4a32e01 Compare December 19, 2018 18:13
@morgante morgante merged commit 86baa96 into master Dec 19, 2018
@aaron-lane aaron-lane deleted the feature/automatic-fixture-network-creation branch April 10, 2019 16:12
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.

None yet

3 participants