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

Update tests, add import for auth backends. #12

Merged
merged 4 commits into from
Aug 29, 2017

Conversation

paddycarver
Copy link
Contributor

Auth backends now use the path as the ID, not the name. This will allow
us to have multiple of the same auth backend mounted on the same vault
server. It will also allow us to match the auth backend using just the
path, instead of the path and the type combination.

This is largely necessary for import, where we only have the ID, so we
need the ID to be able to uniquely identify the resource, which the type
can't do, but the path can.

We also updated the initial test to randomise the path the auth backend
is mounted at, instead of mounting it at a static path, which makes
tests more resilient and less likely to fail due to collisions.

Read now sets all the fields in state.

Auth backends now use the path as the ID, not the name. This will allow
us to have multiple of the same auth backend mounted on the same vault
server. It will also allow us to match the auth backend using just the
path, instead of the path and the type combination.

This is largely necessary for import, where we only have the ID, so we
need the ID to be able to uniquely identify the resource, which the type
can't do, but the path can.

We also updated the initial test to randomise the path the auth backend
is mounted at, instead of mounting it at a static path, which makes
tests more resilient and less likely to fail due to collisions.

Read now sets all the fields in state.
@paddycarver
Copy link
Contributor Author

I'm almost positive I need to do a state migration to account for the change in the ID.

We're using helper/acctest now, so it needs to be vendored.
This was referenced Aug 26, 2017
@apparentlymart
Copy link
Contributor

I think you're right about a state migration being needed here.

That alone doesn't make this a breaking change though, since state migrations happen automatically without any user interaction. Perhaps you meant something else as the breaking change here?

@paddycarver
Copy link
Contributor Author

paddycarver commented Aug 28, 2017

Yeah, anyone that uses vault_auth_backend.foo.id in their config will get a different value than they would have before the PR, which is why I considered it breaking. Agreed state migration is BC.

Granted, that is a pretty small breaking change, but figured it was worth calling out in the CHANGELOG anyways.

@apparentlymart
Copy link
Contributor

Ahh yes, that's correct. I expected that people would use .type for this, but indeed people could be depending on .id so worth noting as a breaking change in the CHANGELOG.

Add a migration that changes our auth backend ID for us, and a test to
ensure it's working correctly.
@paddycarver
Copy link
Contributor Author

Updated with the migration and the test for the migration, this should be ready to go now.

Left a var out of a Fatalf, that's embarrassing.
@paddycarver paddycarver merged commit b7e11f0 into master Aug 29, 2017
@paddycarver paddycarver deleted the paddy_auth_backend_import branch August 29, 2017 20:09
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
…ackend_import

Update tests, add import for auth backends.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants