-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
config: Add base64sha256() function #4899
Conversation
This seems fine to me. I have a third formulation to consider, though: could we have a variant of the This alternative makes things a bit more orthogonal (could be used with |
I like the idea of detecting hex vs bytes, I'm just wondering what would be the percentage of use-cases where you actually want base64-encode hexadecimal representation. My personal humble guess is that user is more likely going to hit a problem when encoding hex while actually wanting to encode raw bytes - i.e. I think either the current I'm taking
as 👍 and will merge this PR during the weekend, unless someone comes up with a reason why not to then. |
Ah, hold on... Maybe I misunderstood. Did you want If yes, then I think it will be hard to explain in the docs, as you mentioned and probably even harder to come up with a good name for such function. |
I think the difficulty of communicating my idea within this PR is sufficient evidence that it's a bad idea. :) And yeah, I was meaning 👍 for implementation, ignoring the merits of this particular design. I'm less sure about whether this is the best design for solving this problem, but I think we just have a set of sub-optimal approaches here and no obvious winner so I'm not going to argue. 😀 |
@@ -80,12 +80,17 @@ The supported built-in functions are: | |||
* `base64encode(string)` - Returns a base64-encoded representation of the | |||
given string. | |||
|
|||
* `sha1(string)` - Returns a SHA-1 hash representation of the | |||
given string. | |||
* `base64sha256(string)` - Returns a base64-encoded represantation of raw |
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.
"representation"
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.
Thanks, fixed.
6ba40a8
to
4509afb
Compare
I guess it will be also helpful to add support for warnings & deprecation for built-in functions, so that if we come up with a better solution in the future, we can communicate that gracefully to users. I'd also say that's out of the scope of this PR and the linked Lambda PR, though. |
4509afb
to
1018af5
Compare
config: Add base64sha256() function
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This will help finishing #3825 and solve the problem I described in #3825 (comment)
I was thinking about the name, considering
base64encodedsha256()
, but it seemed a bit too long and unnecessary to keep "encoded" there. It is very unlikely we'll ever need something likebase64decodedsha256()
. That is why I chosen a simple namebase64sha256
.