-
Notifications
You must be signed in to change notification settings - Fork 57
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
enhancement(redact): Add redactor option #640
Conversation
276b758
to
dd3708e
Compare
src/stdlib/redact.rs
Outdated
} | ||
} | ||
} | ||
|
||
/// Compute the digest of some bytes as hex string | ||
fn hex_digest<T: digest::Digest>(value: &[u8]) -> String { | ||
hex::encode(T::digest(value)) |
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.
would it be better to use base64 encoding? or have another option for the text encoding so the caller can choose between hex and base64?
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.
As a potential user of this, what would you find most useful? My instinct is that base64 would be more common. Maybe we could use the same approach as we use for the filters
argument to this function where additional options could be specified, if needed, for a given redactor like:
redact("my id is 123456", filters: [r'\d+'], redactor: {name: "sha256", encoding: "base64"})
But where you could also just specify:
redact("my id is 123456", filters: [r'\d+'], redactor: "sha256")
For the default behavior (say base64 encoding). What would you think?
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.
My instinct is that base64 would be more common
that is my instinct as well.
1f5e9d0
to
ac55877
Compare
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.
Overall this looks good!
- Can you also update the vector docs (see this example) please?
- Examples look great. Can you add similar unit tests?
vector documentation PR: vectordotdev/vector#19749 |
Awesome, thank you. |
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.
Added one suggestion. Otherwise, this looks good. Thank you!
Head branch was pushed to by a user without write access
16604f2
to
9564364
Compare
Head branch was pushed to by a user without write access
9564364
to
4771bb9
Compare
@tmccombs this looks good to me. Do you think it is ready to merge? |
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.
Happy to see this addition!
@tmccombs it looks like there are some merge conflicts now too when you get a second. |
This option allows you to specify either a custom string, or a hash function to use as the redactor, in addition to the current behavior of the fixed string "[Redacted]". Fixes vectordotdev#633
To specify how to encode hash values.
4771bb9
to
d006c97
Compare
yeah, I think this is ready to merge. |
This option allows you to specify either a custom string, or a hash function to use as the redactor, in addition to the current behavior of the fixed string "[Redacted]".
I'm opening this as a draft to make sure this is a good approach.
If this is the API that is desired, I can add some addition examples and tests.
closes: #633