ptuomola commented on pull request #1187: URL: https://github.com/apache/fineract/pull/1187#issuecomment-676858392
@Nayan, @edcable - many thanks for the feedback - much appreciated! A couple of comments in reply: > 1. You get this error if you recreate your Fineract docker image but you do not recreate your MySQL database ( as mentioned in the [FINERACT-1037](https://issues.apache.org/jira/browse/FINERACT-1037)), this rarely happens and not the real reason for the error. Agreed - this is the scenario I set out to address. In terms of this happening only rarely: would this not happen every time if you run a Dockerized Fineract set-up and you upgrade your Fineract Docker image to the latest one e.g. from Docker Hub? Unless I'm missing something, in such case, all the files stored on the previous image would be lost. > 2. When deployed Fineract in clustered mode but storage pointed to the local file system, instead of to a shared network mounted files system Agree that that would lead to the same issue. In case this is a Dockerised installation, then my PR should fix it as well. But if it is non-Dockerized, I don't think we can solve this in the Fineract distribution, as we have no control over how people set up their local filesystems. Maybe we can point this out in the README though? > 3. In the system configuration if they switch from storage from file system to S3 and then from S3 back to file system ( Fineract does not remember the files was stored in S3 or Filesystem it simply tries to access with present configuration ) I'm not sure that's entirely accurate. As far as I can see, Fineract does remember the configuration that a specific document was stored with - it should be stored in m_document.storage_type_enum. The problem is that the specific logic for downloading the import template ignores this and assumes everything is stored in the local file system (see code in BulkImportWorkbookServiceImpl). As I stated in my JIRA comment, I fully agree that this should also be fixed. > * This PR might only fix problem number two. I think this fixes scenarios 1 and 2 for dockerised set-ups. > * The issue number one is more of DevOps, we should not worry about it. Not sure I understand this. Of course there are other ways to solve this as well (e.g. write some upgrade scripts to copy the files to a new container). But in my view storing data you want to retain directly in the container is not a good idea, and given this PR addresses that and fixes 1, would it not make sense to apply it? > * A most important and possible our user may face is the third one, we need to solve is the third one (I have not tested the third one on the latest codebase, assumed based on my past experience) I agree that the code in BulkImportWorkbookServiceImpl needs to be fixed to respect the stored StorageType. I don't have a test environment for S3 repository so it's not easy for me to do this. I can raise a separate JIRA for this and let's hope someone picks this up... ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
