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 policy_arn to be more in line with vault 0.11.0 #259

Closed
wants to merge 5 commits into from

Conversation

moosilauke18
Copy link

Vault was changed from using arn to use policy_arns and role_arns in hashicorp/vault#4360

Without this change terraform will not be able to read the aws_secret_backend_role properly and assumes it always needs to be updated with policy_arn.

Have not tested the Acceptance tests. But have tested locally to existing vault > 0.11.0 where this issue came up.

@cvbarros
Copy link
Contributor

Hi @moosilauke18 , this appears to be a breaking change by renaming this attribute. In these cases, to give opportunity for users to control the upgrade and breaking changes, it's always best to try and accomodate schema changes.

This link is a very useful resource that provides some best practices when dealing with this type of change:
https://www.terraform.io/docs/extend/best-practices/deprecations.html#provider-attribute-rename

Thus, such renames must be weighted against the cost-benefit of a breaking change.

Regarding the acceptance tests, do you need any assistance to run them locally?

@ghost ghost added size/XL and removed size/M labels Dec 14, 2018
@moosilauke18
Copy link
Author

Added back the deprecated values. Acceptance testing is passing on all except for one import test for policy_document. The old attributes were also failing the import tests. If there is any reading/code examples of how the import actually works, I'd love to read them.

@tyrannosaurus-becks
Copy link
Contributor

@moosilauke18 thanks for working on this! I did recently add a CONTRIBUTING.md to the repo that may help with tests? If you're not finding what you need there (under Testing, in Extending Terraform), let me know and I can hopefully point you to something more or improve the guide.

@tyrannosaurus-becks
Copy link
Contributor

Closing this for now due to inactivity. Please feel free to reopen it if you get a chance to circle back around. Thanks!

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.

3 participants