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

Fixed parameter name for policy_arn setting #49

Merged
merged 1 commit into from
Jan 30, 2018

Conversation

mvisonneau
Copy link
Contributor

@mvisonneau mvisonneau commented Dec 20, 2017

This solves an issue that prevents the use of policy_arn setting on the vault_aws_secret_backend_role resource.

eg:

module.vault_aws.vault_aws_secret_backend_role.admin: Creating...
  backend:    "" => "aws"
  name:       "" => "admin"
  policy_arn: "" => "arn:aws:iam::aws:policy/AdministratorAccess"

Error: Error applying plan:

1 error(s) occurred:

* module.vault_aws.vault_aws_secret_backend_role.admin: 1 error(s) occurred:

* vault_aws_secret_backend_role.admin: Error creating role "admin" for backend "aws": Error making API request.

URL: PUT https://vault.example.com/v1/aws/roles/admin
Code: 500. Errors:

* 1 error occurred:

* Either policy or arn must be provided

@paulRbr
Copy link

paulRbr commented Jan 10, 2018

These changes seem pretty straight forward. Especially when seeing the vault API documentation for the endpoint: https://www.vaultproject.io/api/secret/aws/index.html#create-update-role

LGTM.

@paddycarver
Copy link
Contributor

Hi @mvisonneau, thanks so much for the PR! The change looks great. Do you mind, however, just adding a test case that exercises the broken code? If you don't have time or, understandably, have moved on while waiting for a reply to this, I'll come back at the end of the week and add the test case and push this forward. Thanks again!

@mvisonneau
Copy link
Contributor Author

mvisonneau commented Jan 30, 2018

Thanks for the feedback @paddycarver, I've added the tests.

Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

I think I'd prefer if this had been a new test function, instead of getting added to an existing function, but honestly that's more a stylistic concern, and I'd like to get this fix rolled out. Thanks so much for taking the time to PR this!

@paddycarver paddycarver merged commit 814ab29 into hashicorp:master Jan 30, 2018
@mvisonneau
Copy link
Contributor Author

thanks! :) will do next time

@mvisonneau mvisonneau deleted the policy_arn_role branch January 30, 2018 21:03
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
Fixed parameter name for policy_arn setting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants