Skip to content
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

Add support for archive folder being on SMB share #340

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from

Conversation

robintw
Copy link
Collaborator

@robintw robintw commented Apr 21, 2020

Adds support for the archive folder being located on a Windows (SMB) share. See the updated configuration.rst file in the PR for details - but basically just set the archive path in the config file to something like \\SERVER\share\path\to\folder, along with the right username and password, and it should all work.

This is implemented by wrapper functions that are used whenever we write to the archive folder (principally in the FileProcessor, but also in the HighlightedFile class) - and these dispatch to two different implementations, depending whether the archive is on a SMB share or not. All the SMB functions are wrapped in try...except blocks to deal with authentication/network issues.

Tests are written on the assumption that the smbprotocol library behaves as advertised - and thus the tests only check that the right SMB functions are called. This also means we don't need a SMB server available to run the tests, which makes life a lot easier.

Fixes #189

@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #340 into develop will decrease coverage by 0.03%.
The diff coverage is 94.20%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #340      +/-   ##
===========================================
- Coverage    96.38%   96.35%   -0.04%     
===========================================
  Files           64       65       +1     
  Lines         5868     5918      +50     
===========================================
+ Hits          5656     5702      +46     
- Misses         212      216       +4     
Impacted Files Coverage Δ
pepys_import/file/smb_and_local_file_operations.py 91.83% <91.83%> (ø)
config.py 100.00% <100.00%> (ø)
pepys_import/file/file_processor.py 98.25% <100.00%> (-0.01%) ⬇️
pepys_import/file/highlighter/support/export.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bffc78...222225f. Read the comment docs.

@robintw robintw marked this pull request as ready for review April 22, 2020 15:39
@robintw
Copy link
Collaborator Author

robintw commented Apr 22, 2020

This is now ready for review. The reason for the low coverage for the patch is that we're not testing against a live SMB server, so we can't test that we get particular exceptions raised under specific circumstances (eg. a SMBAuthenticationError when we give the wrong username or password). I don't think there's much we can do about this, unless we decide to go down the route of requiring a live SMB server for the tests.

@robintw robintw requested a review from IanMayo April 22, 2020 15:40
@robintw robintw marked this pull request as draft April 24, 2020 13:53
@robintw robintw marked this pull request as ready for review April 24, 2020 14:11
@robintw
Copy link
Collaborator Author

robintw commented Oct 1, 2021

Update before I leave:

This is an extremely old PR that is basically working, but will probably need some changes to work with the latest version of Pepys. The users said at one point that they wanted all the data to be archived to a SMB (Samba - Windows File Sharing) share using a different username and password to the one the user is actually logged in to. This is so a secure archive of files could be created. The user's priorities changed, and we didn't have chance to properly test it on-site, so it got deprioritised.

This was actually remarkably difficult to do, and required using a pure-Python SMB library rather than the in-built Windows methods. It's all been tested and benchmarks, and so with a few modifications for the latest Pepys it should 'just work'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow archive folder identity to be loaded
2 participants