[plaso] Moved storage implementations

[plaso] Moved storage implementations

Rationale here is primarily to make reviewing storage changes easier (evaluating 4 lines in a 1200+ line file makes it difficult to evaluate the full scope of the change). Other bonus is to surface issues with the storage interface design, and testing gaps.

[Joachim Metz, 2018-01-02 18:11:27]

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_f...
File plaso/storage/sqlite/sqlite_file.py (right):

https://codereview.appspot.com/334410043/diff/1/plaso/storage/sqlite/sqlite_f...
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/t...
File tests/parsers/esedb_plugins/test_lib.py (right):

https://codereview.appspot.com/334410043/diff/1/tests/parsers/esedb_plugins/t...
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#newc...
tests/storage/factory.py:28: self.assertIsInstance(storage_file,
break after ( for consistency

[onager, 2018-01-03 19:33:02]

Code updated.

[onager, 2018-01-03 19:39:10]

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_f...
File plaso/storage/sqlite/sqlite_file.py (right):

https://codereview.appspot.com/334410043/diff/1/plaso/storage/sqlite/sqlite_f...
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/t...
File tests/parsers/esedb_plugins/test_lib.py (right):

https://codereview.appspot.com/334410043/diff/1/tests/parsers/esedb_plugins/t...
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#newc...
tests/storage/factory.py:28: self.assertIsInstance(storage_file,
On 2018/01/02 18:11:27, Joachim Metz wrote:
> break after ( for consistency

Done.

[Joachim Metz, 2018-01-04 15:52:11]

I'll re-review after merge of https://codereview.appspot.com/332510043/

[onager, 2018-01-09 15:01:20]

Code updated.

[onager, 2018-01-09 15:04:09]

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.

[Joachim Metz, 2018-01-09 21:00:33]

please create a PR for this CR as well

[onager, 2018-01-10 13:37:31]

Code updated.

[Joachim Metz, 2018-01-10 20:37:24]

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#newc...
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...
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#...
tests/storage/factory.py:28: self.assertIsInstance(
why the multi line changes? doesn't this fit on a single line?

[Joachim Metz, 2018-01-10 20:38:31]

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#newc...
tests/parsers/bsm.py:297: self.AssertDictContains(extra_tokens_dict,
expected_extra_tokens_dict)
or make the method "protected" _AssertDictContains ?

[onager, 2018-01-11 12:58:04]

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#newc...
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#newc...
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#...
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.

[onager, 2018-01-11 13:51:28]

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