Adding more work on salt encryption #353
Conversation
thomasm-ttd
left a comment
thomasm-ttd
left a comment
There was a problem hiding this comment.
Are you still adding tests for these?
|
|
||
| @Override | ||
| public StoreReader<Collection<RotatingSaltProvider.SaltSnapshot>> getReader(Integer siteId) { | ||
| return null; |
There was a problem hiding this comment.
Should these methods not throw, rather than return null? If we don't expect them to be called, they should at least throw so we catch the error earlier.
Or do we divide the interface so that the methods are not required here?
There was a problem hiding this comment.
Throwing would change the interface too. But I think splitting it up would also be annoying.
I am thinking when we continue with Admin rework, we migrate salts to follow the pattern of our other files and we can fill these in then.
There was a problem hiding this comment.
Ok, lets leave it as, but add logging (at WARN level) to indicate the method is being called.
thomasm-ttd
left a comment
thomasm-ttd
left a comment
There was a problem hiding this comment.
Small comment about adding some logging
…ABTechLab/uid2-admin into cbc-UID2-4571-cloud-encryption-cleanup
…ABTechLab/uid2-admin into cbc-UID2-4571-cloud-encryption-cleanup
No description provided.