Code review - Issue 334410043: [plaso] Moved storage implementationshttps://codereview.appspot.com/2018-01-11T13:51:28+00:00rietveld
Message from unknown
2018-01-02T16:22:47+00:00onagerurn:md5:d50aac96e85780bd2f6f789943600e2f
Message from onager@deerpie.com
2018-01-02T16:23:01+00:00onagerurn:md5:54228c4a8bde67680c4780036e08a0b5
Message from joachim.metz@gmail.com
2018-01-02T18:11:27+00:00Joachim Metzurn:md5:c4ae69796110be34e4c6deaa1311ffde
There are numerous pending CLs that touch the files you are changing. We need to merge them first, then make these changes. I would like to see some dependency information in the CL description to coordinate merging this CL.
https://codereview.appspot.com/334410043/diff/1/.pylintrc
File .pylintrc (right):
https://codereview.appspot.com/334410043/diff/1/.pylintrc#newcode175
.pylintrc:175: ignored-modules=pyesedb
let's move this into the pylint CL
https://codereview.appspot.com/334410043/diff/1/plaso/storage/sqlite/sqlite_file.py
File plaso/storage/sqlite/sqlite_file.py (right):
https://codereview.appspot.com/334410043/diff/1/plaso/storage/sqlite/sqlite_file.py#newcode13
plaso/storage/sqlite/sqlite_file.py:13: from plaso.storage import interface
alphabetical order
https://codereview.appspot.com/334410043/diff/1/tests/parsers/esedb_plugins/test_lib.py
File tests/parsers/esedb_plugins/test_lib.py (right):
https://codereview.appspot.com/334410043/diff/1/tests/parsers/esedb_plugins/test_lib.py#newcode10
tests/parsers/esedb_plugins/test_lib.py:10: from plaso.storage.fake import writer
for consistency:
+ as fake_writer
(repeat across the rest of the changes)
https://codereview.appspot.com/334410043/diff/1/tests/storage/factory.py
File tests/storage/factory.py (right):
https://codereview.appspot.com/334410043/diff/1/tests/storage/factory.py#newcode28
tests/storage/factory.py:28: self.assertIsInstance(storage_file,
break after ( for consistency
Message from unknown
2018-01-03T19:32:59+00:00onagerurn:md5:a6b814e669c8e2976e8de4d05247a28b
Message from onager@deerpie.com
2018-01-03T19:33:02+00:00onagerurn:md5:029d5126df81701270b90567aeffcd5e
Code updated.
Message from onager@deerpie.com
2018-01-03T19:39:10+00:00onagerurn:md5:629966074b80a555a9d24ac88aac8bde
There are indeed other CLs changing this files, but this is based off of current master, and doesn't depend on any other changes.
I'm fine to wait for other in flight CLs you think might collide, but I don't see any major conflicts with
https://codereview.appspot.com/337140043/
https://codereview.appspot.com/332510043/
Which you mention in issue #1620
https://codereview.appspot.com/334410043/diff/1/.pylintrc
File .pylintrc (right):
https://codereview.appspot.com/334410043/diff/1/.pylintrc#newcode175
.pylintrc:175: ignored-modules=pyesedb
On 2018/01/02 18:11:27, Joachim Metz wrote:
> let's move this into the pylint CL
This CL is based off the current master, so it's relevant for this change. This will disappear if the pylint CL goes in first.
https://codereview.appspot.com/334410043/diff/1/plaso/storage/sqlite/sqlite_file.py
File plaso/storage/sqlite/sqlite_file.py (right):
https://codereview.appspot.com/334410043/diff/1/plaso/storage/sqlite/sqlite_file.py#newcode13
plaso/storage/sqlite/sqlite_file.py:13: from plaso.storage import interface
On 2018/01/02 18:11:27, Joachim Metz wrote:
> alphabetical order
Done.
https://codereview.appspot.com/334410043/diff/1/tests/parsers/esedb_plugins/test_lib.py
File tests/parsers/esedb_plugins/test_lib.py (right):
https://codereview.appspot.com/334410043/diff/1/tests/parsers/esedb_plugins/test_lib.py#newcode10
tests/parsers/esedb_plugins/test_lib.py:10: from plaso.storage.fake import writer
On 2018/01/02 18:11:27, Joachim Metz wrote:
> for consistency:
>
> + as fake_writer
>
> (repeat across the rest of the changes)
Done.
https://codereview.appspot.com/334410043/diff/1/tests/storage/factory.py
File tests/storage/factory.py (right):
https://codereview.appspot.com/334410043/diff/1/tests/storage/factory.py#newcode28
tests/storage/factory.py:28: self.assertIsInstance(storage_file,
On 2018/01/02 18:11:27, Joachim Metz wrote:
> break after ( for consistency
Done.
Message from joachim.metz@gmail.com
2018-01-04T15:52:11+00:00Joachim Metzurn:md5:b88d16df18030ea4a92489cceb9a3801
I'll re-review after merge of https://codereview.appspot.com/332510043/
Message from unknown
2018-01-09T15:01:10+00:00onagerurn:md5:0a9b5b9a30875a59f019c4e3bc615bc3
Message from onager@deerpie.com
2018-01-09T15:01:20+00:00onagerurn:md5:7e0ba874e1ce402354d2304b7cde1ec5
Code updated.
Message from onager@deerpie.com
2018-01-09T15:04:09+00:00onagerurn:md5:b10bb3650f516fef60f1fa00ca72f4fa
https://codereview.appspot.com/334410043/diff/1/.pylintrc
File .pylintrc (right):
https://codereview.appspot.com/334410043/diff/1/.pylintrc#newcode175
.pylintrc:175: ignored-modules=pyesedb
On 2018/01/03 19:39:10, onager wrote:
> On 2018/01/02 18:11:27, Joachim Metz wrote:
> > let's move this into the pylint CL
>
> This CL is based off the current master, so it's relevant for this change. This
> will disappear if the pylint CL goes in first.
If the pylint CL doesn't land first, I still need this so pylint doesn't fail.
Message from joachim.metz@gmail.com
2018-01-09T21:00:33+00:00Joachim Metzurn:md5:7105f4ebb84fe0b2573976e66c27b806
please create a PR for this CR as well
Message from unknown
2018-01-10T13:37:28+00:00onagerurn:md5:3615b6eb97b87108d110f314b3d5a790
Message from onager@deerpie.com
2018-01-10T13:37:31+00:00onagerurn:md5:085eeab067329a5e89ce9b5237ab1589
Code updated.
Message from joachim.metz@gmail.com
2018-01-10T20:37:24+00:00Joachim Metzurn:md5:00cd3452b6d849c0ec6b97f90a084711
A couple of small nits, please address before merge. Test are green, so no blockers IMHO, so LGTM.
https://codereview.appspot.com/334410043/diff/60001/tests/parsers/bsm.py
File tests/parsers/bsm.py (right):
https://codereview.appspot.com/334410043/diff/60001/tests/parsers/bsm.py#newcode297
tests/parsers/bsm.py:297: self.AssertDictContains(extra_tokens_dict, expected_extra_tokens_dict)
I opt to not use the unittest naming convention here if you want to differentiate between custom test methods and unittest methods. Maybe TestIfDictionaryContainsKeyValuePairs() (or something that does not resemble the unittest naming scheme)
https://codereview.appspot.com/334410043/diff/60001/tests/parsers/test_lib.py
File tests/parsers/test_lib.py (right):
https://codereview.appspot.com/334410043/diff/60001/tests/parsers/test_lib.py#newcode183
tests/parsers/test_lib.py:183: def AssertDictContains(self, received, expected):
See previous remark
https://codereview.appspot.com/334410043/diff/60001/tests/storage/factory.py
File tests/storage/factory.py (right):
https://codereview.appspot.com/334410043/diff/60001/tests/storage/factory.py#newcode28
tests/storage/factory.py:28: self.assertIsInstance(
why the multi line changes? doesn't this fit on a single line?
Message from joachim.metz@gmail.com
2018-01-10T20:38:31+00:00Joachim Metzurn:md5:3b16dffc40c547538a94db7abf2f2b6d
https://codereview.appspot.com/334410043/diff/60001/tests/parsers/bsm.py
File tests/parsers/bsm.py (right):
https://codereview.appspot.com/334410043/diff/60001/tests/parsers/bsm.py#newcode297
tests/parsers/bsm.py:297: self.AssertDictContains(extra_tokens_dict, expected_extra_tokens_dict)
or make the method "protected" _AssertDictContains ?
Message from head06902@gmail.com
2018-01-10T21:25:35+00:00head06902urn:md5:2a9adcacddb06d09c8b36b839a64c94c
Message from onager@deerpie.com
2018-01-11T12:58:04+00:00onagerurn:md5:2d58a2f588c3164d05f355fff683a614
https://codereview.appspot.com/334410043/diff/60001/tests/parsers/bsm.py
File tests/parsers/bsm.py (right):
https://codereview.appspot.com/334410043/diff/60001/tests/parsers/bsm.py#newcode297
tests/parsers/bsm.py:297: self.AssertDictContains(extra_tokens_dict, expected_extra_tokens_dict)
On 2018/01/10 20:38:31, Joachim Metz wrote:
> or make the method "protected" _AssertDictContains ?
>
Done.
https://codereview.appspot.com/334410043/diff/60001/tests/parsers/bsm.py#newcode297
tests/parsers/bsm.py:297: self.AssertDictContains(extra_tokens_dict, expected_extra_tokens_dict)
On 2018/01/10 20:37:24, Joachim Metz wrote:
> I opt to not use the unittest naming convention here if you want to
> differentiate between custom test methods and unittest methods. Maybe
> TestIfDictionaryContainsKeyValuePairs() (or something that does not resemble the
> unittest naming scheme)
Done.
https://codereview.appspot.com/334410043/diff/60001/tests/storage/factory.py
File tests/storage/factory.py (right):
https://codereview.appspot.com/334410043/diff/60001/tests/storage/factory.py#newcode28
tests/storage/factory.py:28: self.assertIsInstance(
On 2018/01/10 20:37:24, Joachim Metz wrote:
> why the multi line changes? doesn't this fit on a single line?
Done.
Message from onager@deerpie.com
2018-01-11T13:51:28+00:00onagerurn:md5:c505b7a7b8a83808f233a499349ec96b
Changes have been merged with master branch. To close the review and clean up the feature branch you can run: review.py close storage_move