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

Enable binary acceptance test driver #99

Merged
merged 4 commits into from
Mar 15, 2020
Merged

Enable binary acceptance test driver #99

merged 4 commits into from
Mar 15, 2020

Conversation

kmoe
Copy link
Member

@kmoe kmoe commented Feb 17, 2020

This PR enables the new binary acceptance test driver available in v1.7.0 of the plugin SDK.

Provider acceptance tests now run real terraform CLI commands under the hood. The test API remains the same, and there should be no other changes to test results or output. Please see hashicorp/terraform-plugin-sdk#262 for more details.

In addition, this PR includes the following changes:

  • Upgrade to SDK v1.7.0.
  • Force use of Go vendoring via Makefile. Prior to this, the vendor/ directory was ignored, and Go modules were downloaded anew during each build, as can be seen in old Travis logs. Please see this memo for more details regarding this change.
  • Add a step to Travis CI to install Terraform prior to the build. This is currently needed by the new binary test driver; subsequent versions may install Terraform automatically.
  • Run make testacc during the Travis build, since this provider's acceptance tests are very quick and do not rely on any external APIs.

@ghost ghost added the size/XXL label Feb 17, 2020
@kmoe kmoe force-pushed the kmoe-binary-testing branch from c36d841 to c6550a1 Compare February 18, 2020 18:21
@kmoe kmoe force-pushed the kmoe-binary-testing branch 2 times, most recently from f388f6f to 3c09d1b Compare March 2, 2020 14:35
@kmoe kmoe requested a review from a team March 2, 2020 15:18
@kmoe kmoe marked this pull request as ready for review March 2, 2020 15:18
GNUmakefile Outdated
@@ -3,6 +3,9 @@ GOFMT_FILES?=$$(find . -name '*.go' |grep -v vendor)
WEBSITE_REPO=github.com/hashicorp/terraform-website
PKG_NAME=random

.EXPORT_ALL_VARIABLES:
GOFLAGS=-mod=vendor
Copy link
Contributor

@appilon appilon Mar 2, 2020

Choose a reason for hiding this comment

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

I wasn't sure myself on enforcing the use of the vendor folder for local development. I would say the vendoring is a relic of speeding up builds for CI in a pre go cache world. I had been actually enforcing modules on just in Travis with GO111MODULE=on GOFLAGS=-mod=vendor. It's something we likely want to harmonize but we can figure that out another day, I'm happy to merge anything that "just works"

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree vendoring is a bigger discussion. At the moment, most providers are using vendoring, at least on TeamCity where it is used for nightly test and release builds. I think we should try to make the development environment match the test/release environment as much as possible, which means that for the moment while vendoring is still being used in TC, it should be used for local development. Since Travis uses the Makefile, setting GOFLAGS here brings both local dev and PR test environments into alignment with TC.

If we remove vendoring in the future, we will have to touch either the Makefile or .travis.yml, wherever we have set GOFLAGS=-mod=vendor (the build will error if there is no vendor/ directory, so we can't leave it in harmlessly).

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, I think I've persuaded myself: whatever we decide to do about vendoring, it's not really related to this PR. Will remove this commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with how this shook out, but wanted to know that our official internal guidance at the moment is to use vendoring when using Go Modules, and deciding against doing that should probably be an RFC.

.travis.yml Outdated
@@ -4,7 +4,10 @@ services:
- docker
language: go
go:
- "1.11.x"
- "1.13.x"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please bump the version of go in .go-version to match so releases and accetance tests on TeamCity remain consistent

Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't need to be bumped, so I've reverted it.

@kmoe kmoe force-pushed the kmoe-binary-testing branch from b0c69bc to 8f3b849 Compare March 5, 2020 14:44
kmoe added 4 commits March 5, 2020 15:42
This commit enables the new binary test driver introduced in SDK v1.7.0 for all acceptance tests in this provider. No test results should change.

Please see hashicorp/terraform-plugin-sdk#262 for more details about this test driver.
@kmoe kmoe force-pushed the kmoe-binary-testing branch from 8f3b849 to 0ebdeba Compare March 5, 2020 15:42
@kmoe kmoe requested a review from appilon March 5, 2020 15:58
@kmoe kmoe merged commit 5f827cd into master Mar 15, 2020
@kmoe kmoe deleted the kmoe-binary-testing branch March 15, 2020 10:03
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants