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

Use new semantic version checking for Consul secrets backend logic #1593

Merged
merged 16 commits into from
Sep 7, 2022

Conversation

robmonte
Copy link
Member

Previously we used a workaround to check which Consul policies field was needed by peeking at the Vault response. With semantic version checking we can more simply get which version of Vault is being used, then reconcile that with the field provided by the user.

For backwards compatibility, both policies and consul_policies work regardless of Vault version, 1.11 being where the field name changed in Vault, but the provider handles mapping the data given by the user to the correct parameter name for Vault depending on version.

Release note for CHANGELOG:

NONE

Output from acceptance testing:

❯ make testacc TESTARGS='-run=TestConsulSecretBackend'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test -run=TestConsulSecretBackend -timeout 30m ./...

ok      github.com/hashicorp/terraform-provider-vault/vault     11.553s

 

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

@robmonte
Copy link
Member Author

Note that I have a gofmt commit before my feature commits.

codegen/generate.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role.go Outdated Show resolved Hide resolved
internal/semver/semver.go Outdated Show resolved Hide resolved
vault/gcp.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role.go Outdated Show resolved Hide resolved
Copy link
Contributor

@vinay-gopalan vinay-gopalan left a comment

Choose a reason for hiding this comment

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

LGTM!

@robmonte
Copy link
Member Author

robmonte commented Aug 31, 2022

I'm not sure what is causing the CI failures now, is it from the fmt issues?
It was unrelated.

@benashz benashz added this to the 3.9.0 milestone Sep 1, 2022
@github-actions github-actions bot added size/L and removed size/M labels Sep 1, 2022
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

a few minor nits to address then 👍

vault/resource_consul_secret_backend.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role.go Show resolved Hide resolved
vault/resource_consul_secret_backend_role_test.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role_test.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role_test.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role_test.go Outdated Show resolved Hide resolved
Each test conditionally runs on Vault version. A new custom ImportStateCheck function is needed to check importing of policies as the provider now always returns consul_policies for an import
@github-actions github-actions bot removed the size/L label Sep 6, 2022
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

A few minor suggestions then 👍

testutil/testutil.go Show resolved Hide resolved
vault/resource_consul_secret_backend_role_test.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend_role_test.go Outdated Show resolved Hide resolved
@robmonte robmonte merged commit d430a3a into main Sep 7, 2022
@robmonte robmonte deleted the semver-consul branch September 7, 2022 20:13
marcboudreau pushed a commit to marcboudreau/terraform-provider-vault that referenced this pull request Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants