-
Notifications
You must be signed in to change notification settings - Fork 540
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
Define "vault_consul_secret_backend_role" resource #480
Define "vault_consul_secret_backend_role" resource #480
Conversation
Hi @p0pr0ck5 ! This PR needs a couple more things before it's ready for review - mainly, tests, and docs. It's more fully outlined here. Happy to circle back and take a look when those are good to go. Thanks for your work on this so far! Definitely interested in getting coverage onto this Consul endpoint. |
Thanks @tyrannosaurus-becks! I will work on these. In the interim, can you let me know if this is expected?
|
@p0pr0ck5 strange! Must have been happening on the version of master from which your branch was created. It's not happening on the current master branch because if it were, tests would fail. Should be resolvable by either pulling master and merging it in, or running |
Weird, still seeing it when keeping up with this project's master:
I know it's not really related to this PR so I'm not super concerned. |
@p0pr0ck5 ah! I bet we have different go versions. Locally I'm on Go 12.7 and CircleCI is on 12.5. I've noticed that when moving to new versions of Go, I've seen differences like that - things that once were considered properly formatted weren't any longer. |
fc5b1f6
to
0aafc77
Compare
Any update on this one? |
Closing due to lack of activity. Thank you for your work, though, and feel free to reopen it if you get a chance to circle back and fix the tests. |
@tyrannosaurus-becks I updated this PR with documentation and tests 20 days ago. The CI run failed due to a timeout grabbing some Go dependencies. Can this please be re-opened and reviewed? |
Specifically the test failure is:
|
@p0pr0ck5 reopened it! Ah yes, the thrift failure, sorry I missed that. It's since been resolved on master. I will merge it in and run tests to see if they now pass. Thank you! |
No worries, I force-pushed this without leaving a comment here, should have communicated that better. Thanks so much! |
Hi @p0pr0ck5 ! OK, so I merged in master and I found I'm still getting some test failures:
Would you be willing to fix those issues, and merge in master and push it up? Happy to circle back around when the tests are green. I'm aiming to do a release in the next week or two, so if it's updated soon enough I may still be able to include it. |
0aafc77
to
8af6e4f
Compare
c28f895
to
73f06d3
Compare
@tyrannosaurus-becks sorry for the noise here. Tests are passing now :) |
Ping @tyrannosaurus-becks - this should be ready for review. Would love to be able to make an upcoming release if possible. Please let me know if anything else is needed! Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic! I'll make sure it gets merged before the upcoming release.
…role Define "vault_consul_secret_backend_role" resource
The commit provides a new resource to manage roles associated with a Consul secrets engine (https://www.vaultproject.io/api/secret/consul/index.html#create-update-role).
This is a bare-bones effort to start a feedback process; I'm not sure if there's a desire to support Consul < 1.4 for this provider.