willmcgugan / codereviews

Free code reviews for open source projects
113 stars 3 forks source link

[Code review request] Pydantic-Vault #10

Open nymous opened 2 years ago

nymous commented 2 years ago

What is the name of your project? Pydantic-Vault

What is the repo URL? https://github.com/nymous/pydantic-vault

Is there anything else I should know about the code? I don't think so ^^ Hope the docs explain what it does, why it's helpful and how to use it

Are there any areas you would like me to focus on? I had a lot of trouble thinking about the API to offer, and especially the priority of the different layers of configuration. Because the lib tries to be helpful in development and in production, it can be configured from environment variables or from the code. I am still not 100% certain of which should override the other if both the env variable and the code want to specify the config value, or which authentication method should win if multiple are found (like if I have a .vault-token locally but also happen to have an Approle ROLE_ID and SECRET_ID env variables defined).

Also, I'm not sure my logging and error handling is ok. Should I error out at the start if the vault_url is not defined, even though I don't error at all if I cannot login to Vault? Should the logs be info, warning... ? Is my way of logging even the correct way?

Also also, I'm not quite happy with the repetition in the _get_authenticated_vault_client and the various _extract_* methods :sweat_smile: I would like to make it easy to extend and provide other authentication methods (like Google Cloud, AWS, Azure, username/password... See the Vault docs for a full list of supported authentication methods).

Finally, are my tests any good? I mock a lot of things for now (about everything communicating with Vault through hvac), as setting up integration tests will be hard. It would need to run a Vault server, put some data in it, and maybe even run a Kubernetes cluster to make sure its authentication works (not even talking about the future integration with cloud providers...).

Have I reviewed this project previously? No

Thank you for offering this service :heart: