Conversation
| @@ -31,8 +32,12 @@ pub struct RedisCache { | |||
| impl RedisCache { | |||
| /// Create a new `RedisCache`. | |||
| pub fn new(url: &str) -> Result<RedisCache> { | |||
| let mut parsed = Url::parse(url)?; | |||
| if parsed.password().is_some() { | |||
| let _ = parsed.set_password(Some("*****")); | |||
There was a problem hiding this comment.
nit: I would prefer a wrapper type, that has an alternative Display implementation rather than this hack. At the very least this requires a comment to why this is done and a statement of the restrictions following from it.
There was a problem hiding this comment.
Hack? I would consider this the simplest possible implementation for masking the password for display.
The url member of RedisCache is used only for display purposes. How about I rename this member to displayUrl and add a comment?
There was a problem hiding this comment.
My apologies, "hack" was definitely the wrong choice of words. There is just the possibility of this being re-used in the future and not being explicit is a potential future pain point.
Renaming to display_url surely conveys this, though I would still prefer this to be generalized to a RedactedUrl impl eventually beyond the scope of the redis storage backend, but out of scope for this PR.
There was a problem hiding this comment.
No worries.
Comment added.
|
LGTM, minor nit needs to be addressed, other than that 👍 - thank you! ❤️ |
alnr
force-pushed
the
redis-tls-cachepot
branch
from
fe952ae to
f717bf5
Compare
|
Looks good, thank you! |
3 participants
The corresponding PR in mozilla/sccache is here: mozilla/sccache#976
This enables TLS support for Redis. Just by fiddling with the features of the redis dependency,
Cargo.lockshrank a lot.I've also added a small improvement (masking the password in the Redis URL if there is one).