Run testacc in CI, fix broken tests.

[Mongey]

• Bumps the version of go to 1.10
• Starts a Vault server in CI, runs the acceptance tests against it.
• Fixes various broken tests

🚧 🏗 while I figure out what to do about the bound_* schema change in the vault_aws_auth_backend_role to lists.

[tyrannosaurus-becks]

@Mongey I'm really, really excited about this PR. Apologies for it sitting for so long, this repo was transitioning from the Terraform to the Vault team and I'm just getting started with it and digging through the backlog.

I'm running the acceptance tests against Vault 0.11.1 and I have Go 1.11 installed. Many are passing that weren't before, but there are a couple of failures. Any chance you'd be willing to circle back to this and fix the remaining ones against the same versions?

With that, I'd be happy to approve and merge this PR.

[Mongey]

@Mongey I'm really, really excited about this PR. Apologies for it sitting for so long, this repo was transitioning from the Terraform to the Vault team and I'm just getting started with it and digging through the backlog.

No worries, thanks for taking a look 🙌

I'm running the acceptance tests against Vault 0.11.1 and I have Go 1.11 installed. Many are passing that weren't before, but there are a couple of failures. Any chance you'd be willing to circle back to this and fix the remaining ones against the same versions?

After starting the Vault server you need to manually mount a v1 kv backend with

vault secrets enable --version=1 -path=secretsv1 kv

This isn't the ideal solution, I'd actually prefer to do the mount within the tests itself, but it's not currently possible to create mounts, with options (yet). I can create a follow up PR to do that.

make testacc is passing in CI now

With that, I'd be happy to approve and merge this PR.

🏆
I'm still not sure if the changes to vault_aws_auth_backend_role are 💯. I've added a state migration + tests, but I'm thinking that the attribute names should be updated to use the plural form, rather than just changing the value to a list.
e.g. bound_ami_id -> bound_ami_ids

[tyrannosaurus-becks]

On changing bound_ami_id -> bound_ami_ids, might as well leave it as-is because it matches how Vault has them so it'll be easiest to recognize them. My hope, anyways, is that more people are using the IAM auth method because it came later and is super secure when used with credentials like those found in instance metadata.

I looked it all over and it's looking great! Thank you!

[idubinskiy]

@tyrannosaurus-becks It seems as though the changes to the vault_aws_auth_backend_role attributes here are not backwards-compatible, causing errors like vault_aws_auth_backend_role.aws_role: bound_iam_principal_arn: should be a list on configurations that worked with the previous release of the provider.

[tyrannosaurus-becks]

@idubinskiy hi! Yes, there were quite a lot of changes in Vault since the Terraform provider was last moving forward. Vault itself has also had some breaking changes, though we try so hard to minimize those.

We were thinking that when those happen, our main goal would be to just stay in sync with the most current version and ask the community to only upgrade if they're ready to make those changes. I'm new to being the approver on this repo. Would you suggest a different approach? Definitely open to your thoughts.

[idubinskiy]

@tyrannosaurus-becks It's not that breaking changes should never be made, but they definitely shouldn't be made in patch releases. The Terraform docs suggest users set provider versions using the ~> constraint "to ensure that new versions with breaking changes will not be automatically installed by terraform init in future". In this case, this change was released as a patch and broke out ability to deploy our services. We ended up having to hardcode the version directly to 1.1.2, which is obviously not ideal.

In other TF providers, such as the AWS provider, I've seen the maintainers be very hesitant about making breaking changes to existing resources or attributes. Often the solution is to deprecate the old attribute and create a new one, such as placement_strategy and ordered_placement_strategy here.

My suggestion in this case is to cut a new patch version of this resource which reverts the breaking changes introduced by this PR, and then follow with a minor release that includes new attributes and deprecates (but doesn't remove) the old attributes. The deprecated attributes could be removed in a future major version release (2.0) or a future minor version release, ideally after people have had enough time to fix their usage.

[dhild]

I’d just like to echo my support for the strategy @idubinskiy suggests. The plural naming that @Mongey presents feels like a more canonical terraform approach to me.

Incidentally, our strategy for dealing with multiple values in these fields has been to use the join function, which Vault has been accepting and doing it’s own type munging on.

[tyrannosaurus-becks]

@idubinskiy thanks for that explanation! That was very helpful. My apologies for the impact the release had, I'm learning about Terraform providers here. I'll work on just what you've suggested today. @dhild thanks for your comments as well.

[idubinskiy]

@tyrannosaurus-becks Happy to help! Thanks for cutting the reverted release.