Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[match] improve encryption internals, solving flaky test (#21663) #21790
[match] improve encryption internals, solving flaky test (#21663) #21790
Changes from 11 commits
f6aeb5c
1828882
4e68ebb
c5d33cd
e5b271a
4332cc9
8c04014
e07a6f2
7170033
91f0b00
133d090
471c3a0
cb496ec
850a472
0d864ca
0394f34
035cbd1
10405ee
237adc0
56578d8
7ed2e06
4c6f360
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Shouldn't this be
bin/match_file
instead ofmatch_file
?Also, this looks like a typo:
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.
I think that once you install the gem, everything under
bin
will be in your CLASSPATH. SOmatch_file
should be enough.The [] were because the second parameter is optional and match_file decrypts / encrypts in place when omitted.
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.
ooh 😮 gotcha. I didn't know about any of those 2 info you said haha good to know 💪
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.
I considered increasing this very low iteration value, but changing the value requires changing the encryption format anyway, so let's just change the cipher etc.
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.
Just as a suggestion but nothing blocking, I think this could be refactored so that the IV gen happens in one class per algorithm, and then there's a single encrypt/decrypt function that takes in the IV gen class. I think overall it would just cut down on the duplication a little bit.
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.
You mean as passing the ivgen function as a "strategy" to the same code that is shared between V1 and V2?
I thought about it, but decided against for now because
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.
Another question I have: do we want to enforce keywords parameters instead?
I think it usually makes for better API management and backwards compatibility.
Should I update this file and use keywords args instead?
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.
I'm all for keyword parameters whenever we can 🙏
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.
I added some but not in all classes. Makes sense or inconsistent?