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

add base64 support #40

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@bryanhuntesl
Copy link

bryanhuntesl commented Jan 10, 2019

Hi there - it's useful when using a crypto lib like https://github.com/bryanhuntesl/cloak_ecto to have the base64 encoded key decoded / validated by the confex - hope this is of use to you.

@AndrewDryga

This comment has been minimized.

Copy link
Member

AndrewDryga commented Jan 10, 2019

Hello, @bryanhuntesl! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@@ -30,6 +30,14 @@ defmodule Confex.Type do
{:ok, value}
end

def cast(value, :base64) do
try do

This comment has been minimized.

@AndrewDryga

AndrewDryga Jan 10, 2019

Member

Prefer using an implicit try rather than explicit try.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 10, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 187d664 on bryanhuntesl:feature/add/base64 into 903632d on Nebo15:master.

bryanhuntesl added some commits Jan 10, 2019

@bryanhuntesl

This comment has been minimized.

Copy link

bryanhuntesl commented Jan 10, 2019

Fails on Elixir: 1.4.5 OTP Release: 20 - because on that specific version the error message when trying to decode bad string is "incorrect padding" rather than "non-alphabet digit found: \"$\" (byte 36)" - what do you suggest? - fixed using version matching - 2358e44#diff-53ba0e88991c38dff1d963e689cf0c88R17

bryanhuntesl added some commits Jan 10, 2019

@AndrewDryga

This comment has been minimized.

Copy link
Member

AndrewDryga commented Jan 11, 2019

Hello @bryanhuntesl, thanks for PR. I'm sorry to say this but looks like we can't accept this PR. If we keep adding casting for all kinds of non-native formats (integer, charlist, list, etc are native ones), then Confex would quickly become too big and too complex.

In this particular case I would use custom type resolved mentioned in README:

Type can be replaced with {module, function, arguments} tuple, in this case Confex will use external function to resolve the type. Function must returns either {:ok, value} or {:error, reason :: String.t} tuple.

If it doesn't work for you - please tell why and we'll find a way to solve your use case.

@bryanhuntesl

This comment has been minimized.

Copy link

bryanhuntesl commented Jan 11, 2019

Hello @bryanhuntesl, thanks for PR. I'm sorry to say this but looks like we can't accept this PR. If we keep adding casting for all kinds of non-native formats (integer, charlist, list, etc are native ones), then Confex would quickly become too big and too complex.

In this particular case I would use custom type resolved mentioned in README:

Type can be replaced with {module, function, arguments} tuple, in this case Confex will use external function to resolve the type. Function must returns either {:ok, value} or {:error, reason :: String.t} tuple.

If it doesn't work for you - please tell why and we'll find a way to solve your use case.

I'll give that a try - just for background - it's for including cloak_ecto key in config - it always needs to be base64 encoded and supplied as an environmental variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment