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

Consul secret backend #59

Merged
merged 13 commits into from
Sep 18, 2018
Merged

Conversation

draggeta
Copy link
Contributor

@draggeta draggeta commented Jan 26, 2018

To fix one of the issues mentioned in #52, I've created this pull request. The code functions, but there are still some areas I'm unsure of due to lack of knowledge of Go (no prior experience except for PowerShell). It is mostly based on the AWS secret backend resource.

Similar to the AWS secret backend, it mounts the backend as well. This is due to the fact that the configuration cannot be removed once configured. If you want to remove the configuration, the whole mount needs to be removed.

Areas I'm unsure of:

  • I can't seem to find some newby friendly documentation on testing Terraform resources. The tests seem to be done now and finally pass the local acceptance tests. However, if anyone has any newby friendly guides, please send them over :)
  • Only contains the backend configuration for now. After reading the community guidelines, it seemed best to leave the roles for if this gets merged.

Tony Fortes Ramos added 3 commits January 30, 2018 01:45
- passes acceptance tests
- incorporated backend name change to path
- parameterized the terraform configs a little bit
@draggeta
Copy link
Contributor Author

Tests seem to be working now I think.

@paddycarver paddycarver self-assigned this Feb 8, 2018
@paddycarver paddycarver self-requested a review February 8, 2018 23:58
@draggeta
Copy link
Contributor Author

Is anything on the Vault Terraform provider still active? Nothing has been merged since may 7th. That is more than 2,5 months ago.

Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

Hi @draggeta ! Apologies for my slowness in getting to this, I'm just getting ramped up. This code looks great. I ran the acceptance tests and they work. My comment about stripping the field type declaration is a non-blocker. I was just wondering if you could merge in the latest master and push it, and if so, I'd be happy to approve and merge this.

vault/resource_consul_secret_backend.go Outdated Show resolved Hide resolved
@tyrannosaurus-becks tyrannosaurus-becks self-assigned this Sep 14, 2018
@ghost ghost added the size/L label Sep 17, 2018
@draggeta
Copy link
Contributor Author

Hi @tyrannosaurus-becks. Thanks for reviewing it. That was indeed an unnecessary type declaration. Still new to the language. I've removed it and fixed the merge conflicts. Hopefully it's suited now for merging.

Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

Thank you! Excellent work. We appreciate your contribution.

@tyrannosaurus-becks tyrannosaurus-becks merged commit 7eb86cc into hashicorp:master Sep 18, 2018
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
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