search

bitc / hs-vault-tool

Haskell client library for HashiCorp's Vault tool (via HTTP API)
10 stars 9 forks source link

Added AppRole types and functions to allow connecting to vault via Ap… #1

Closed mdunnio closed 6 years ago

mdunnio commented 6 years ago

…pRole credentials.

bitc commented 6 years ago

Thanks for this!

  1. Do you think that VaultAppRoleId and VaultAppRoleSecretId should be "Text" instead of "ByteString"? From what I can see they are always used in JSON strings so it doesn't make sense for them to ever be binary data.

  2. Do you think you could add a test? The tests are in: https://github.com/bitc/hs-vault-tool/blob/master/vault-tool-server/test/test.hs They are a bit messy, but it should be pretty simple to add a test for the new functions you added. I can still merge this without tests if this is urgent for you

mdunnio commented 6 years ago

Do you think that VaultAppRoleId and VaultAppRoleSecretId should be "Text" instead of "ByteString"? From what I can see they are always used in JSON strings so it doesn't make sense for them to ever be binary data.

I think you're right. I can't find anywhere in the documentation where I need to include the VaultAppRoleId and VaultAppRoleSecretId as headers. I'll change to Text.

Do you think you could add a test? The tests are in: https://github.com/bitc/hs-vault-tool/blob/master/vault-tool-server/test/test.hs They are a bit messy, but it should be pretty simple to add a test for the new functions you added. I can still merge this without tests if this is urgent for you

Let me see what I can put together.

Thanks for the response and feedback.

mdunnio commented 6 years ago

@bitc I updated the PR. Changed the types as you mentioned and wrote some tests. Let me know what you think. Thanks.

bitc commented 6 years ago

Thanks, I'll merge this and release a new version on hackage :+1: