-
Notifications
You must be signed in to change notification settings - Fork 1
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
Zero downtime 2.0 #35
Conversation
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.
@nattfodd looks great indeed — in fact it might be more useful if we separate the data migration scheme and the attr_encryption “processor”, for example, so when one day we switch to another non-attr_encryption gem, we can still take advantage of the data migration scheme!
Do you think it fits the scope of |
Yes that’s what I meant too. Feel free to proceed. |
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.
Looks good!
@nattfodd I believe these enhancements can be implemented after this PR is merged:
|
let(:id) { 3 } | ||
|
||
it 'does not change encrypted field' do | ||
expect { MigrationSpec.find(id) } |
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.
Lint/AmbiguousBlockAssociation: Parenthesize the param change { encrypted_columns_for.call(id) } to make sure that the block will be associated with the change method call.
key: '94dd7e2c40a3d51a8dd0a9137356a18e', | ||
algorithm: 'RC2-64-CBC' | ||
}, | ||
version: 20180401000002 |
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.
Style/NumericLiterals: Use underscores(_) as decimal mark and separate every 3 digits with them.
key: '67c3800d1572d9d964a6ff3bd821ed02', | ||
algorithm: 'aes-256-gcm' | ||
}, | ||
version: 20180401000000 |
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.
Style/NumericLiterals: Use underscores(_) as decimal mark and separate every 3 digits with them.
end | ||
end | ||
|
||
context 'when migration is used' do |
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.
Metrics/BlockLength: Block has too many lines. [43/25]
let(:id) { 1 } | ||
|
||
it 'does not migrate encrypted fields' do | ||
expect { MigrationSpec.find(id) } |
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.
Lint/AmbiguousBlockAssociation: Parenthesize the param change { encrypted_columns_for.call(id) } to make sure that the block will be associated with the change method call.
algorithm: 'RC2-64-CBC' | ||
end | ||
|
||
describe Transcryptor::Migration do |
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.
Metrics/BlockLength: Block has too many lines. [78/25]
def specify_latest_version! | ||
migrated_fields = Transcryptor::Migration.migrations[self.class] | ||
migrated_fields.each do |field, versions| | ||
latest_v = versions[:latest_version].to_i |
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.
Layout/ExtraSpacing: Unnecessary spacing detected.
Layout/SpaceAroundOperators: Operator = should be surrounded by a single space.
# user.ssn_20180401000000 | ||
# user.ssn_20180401000001 | ||
# user.ssn_20180401000002 | ||
def patch_models! |
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.
Metrics/MethodLength: Method has too many lines. [11/10]
ed68f6a
to
ba15da2
Compare
I believe the PR is ready to be merged (besides those nasty hound comments, which is difficult to fix). Having direction from unencrypted to encrypted and vice versa isn't still clear for me, since encrypted field is called Let's discuss it in a different Pull Request when it will be subject to discuss. |
@nattfodd sounds good, please feel free to merge it with rebase. Regarding unencrypted to encrypted, I suspect the following naming pairs make better sense?
So the database will:
Perhaps something like this is reasonable? |
I don't have access to write to the repository :) |
Hmm.. I checked settings and it shouldn't be the case though. Anyway I'll merge it here. |
Ha, just found out why 😉 Fixed. |
This pull request addresses the following issues: #33, #24
Idea
To make this happen you need to create a column, that contains current version of the encrypted field. For example, if you have
attr_encrypted :ssn
, you need to create in your table another column:ssn_version
, with some default value - let it be20180401000000
for example.Assuming, you want to change encryption now. You create a new initializer file
config/initializers/transcryptor.rb
. And describe there the history of your changes:Complete example of usage can be found here: https://github.com/nattfodd/transcryptor_sample_app/pull/1/files
Pros:
Transcryptor
)***_version
field to perform as many migrations, as needTODOs:
attr_encrypted
default behaviour