Cleanup duplicated data config paths #4314

cmadjar · 2019-02-08T16:55:26Z

Brief summary of changes

This PR cleans up all the different paths that are referring to the same path (a.k.a. /data/%PROJECT%/data/. This gets rid of the Config settings mincPath and data and updates the code accordingly.

The code changes on the LORIS-MRI front can be found in aces/Loris-MRI#399

This resolves issue...

Github nifti files are stored under mincpath #4197 and LORIS-MRI nifti files are stored under mincPath Loris-MRI#367

To test this change...

run the SQL patch
make sure the imaging modules work properly and are able to see the images
test the API v0.0.2 and v0.0.3-dev to make sure the images can be found and read

Caveat for existing projects

Make sure that your Config setting dataDirBasepath and imagePath are configured to the proper data path (typically /data/%PROJECT%/data). Note that dataDirBasepath is the path used by the LORIS-MRI pipelines and imagePath is the path where the imaging data can be read by the web server.

SQL/0000-00-03-ConfigTables.sql

SQL/New_patches/2019-02-08_cleanup_duplicated_data_path_in_Config.sql

driusan · 2019-02-11T14:43:01Z

SQL/0000-00-03-ConfigTables.sql

@@ -133,7 +130,7 @@ INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType,

 -- Loris-MRI/Imaging Pipeline options from the $profile (commonly "prod") file
 INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, Label, OrderNumber) VALUES ('imaging_pipeline', 'Imaging Pipeline settings', 1, 0, 'Imaging Pipeline', 12);
-INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'dataDirBasepath', 'Base Path to the data directory of Loris-MRI', 1, 0, 'text', ID, 'Loris-MRI Data Directory', 1 FROM ConfigSettings WHERE Name="imaging_pipeline";
+INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'dataDirBasepath', 'Base Path to the data ' || 'directory of Loris-MRI', 1, 0, 'text', ID, 'Loris-MRI Data Directory', 1 FROM ConfigSettings WHERE Name="Paths";


The description of this should specify whether this is relative to the imaging server or the web server (and the type will need to be changed once #4023 is merged)

At the last imaging meeting, we discussed we would only keep one setting so technically this one would work for both. We can bring it up again at the next imaging meeting. I'll add it to the agenda.
Thanks!

But it can't work for both imaging pipeline and web server, the paths are different if they're mounted on different places on the different servers

@driusan in the next commit I reintegrated the imagePath for the apache server and kept the dataDirBasepath for the LORIS-MRI server. Please re-review

johnsaigle · 2019-02-11T16:20:49Z

Travis is failing because of exception handling from this PR https://github.com/aces/Loris/pull/4165/files. If the settings there have been deleted, make sure to remove them from the checks. That should satisfy Travis. :)

cmadjar · 2019-02-11T22:24:19Z

@PapillonMcGill Thanks! I fixed your comments

cmadjar · 2019-02-11T22:34:47Z

@johnsaigle I am confused. I have not touched the config settings mentioned in https://github.com/aces/Loris/pull/4165/files so there should not be an issue. I only removed a few path config settings and cannot grep in the code for the old config settings so I am not sure where it gets it from.
Travis started again so I cannot look for the error message. :S. Will check again later.

johnsaigle · 2019-02-12T17:36:55Z

@cmadjar Hm yeah I see your point. That function will catch any ConfigurationException that occurs so maybe there is something else going on? Are you able to access dicom_archive on your VM?

LGTM

driusan · 2019-02-25T17:04:32Z

I don't see how this is going to work when the web server and imaging pipeline server don't have the same data dir mounted in the same location.

Don't we need 2 settings, one relative to each?

cmadjar · 2019-02-25T18:10:33Z

@driusan I did the change you requested and there is indeed two paths remaining: one for the webserver and one for the imaging pipeline.

I forgot to update the description of the PR and apparently to add again the imagePath into SQL/0000-00-03-ConfigTables.sql . Should be fixed now. Please re-review. Thanks!

driusan · 2019-02-25T18:52:24Z

@cmadjar sorry, I'm still confused.. I'm just looking at the diff in SQL/0000-00-03-ConfigTables.sql and I only see one path being added for 'dataDirBasepath'. What's the other one?

cmadjar

@driusan See my comments directly on 0000-00-03-ConfigTables.sql to answer your question.

cmadjar · 2019-02-25T19:07:17Z

SQL/0000-00-03-ConfigTables.sql

@@ -230,7 +226,7 @@ INSERT INTO Config (ConfigID, Value) SELECT ID, "Produced by LorisDB" FROM Confi
 INSERT INTO Config (ConfigID, Value) SELECT ID, "S3cret" FROM ConfigSettings WHERE Name="JWTKey";


-INSERT INTO Config (ConfigID, Value) SELECT ID, "/PATH/TO/DATA/location" FROM ConfigSettings cs WHERE cs.Name="Loris-MRI Data Directory";
+INSERT INTO Config (ConfigID, Value) SELECT ID, "/data/%PROJECTNAME%/data/" FROM ConfigSettings cs WHERE cs.Name="dataDirBasepath";


@driusan in this line, I modified the default path of dataDirBasepath but I did not add it (the line in red above is for the same config setting)

cmadjar · 2019-02-25T19:07:57Z

SQL/0000-00-03-ConfigTables.sql

@@ -187,9 +185,7 @@ INSERT INTO Config (ConfigID, Value) SELECT ID, "YMd" FROM ConfigSettings WHERE

 INSERT INTO Config (ConfigID, Value) SELECT ID, "/data/%PROJECTNAME%/data/" FROM ConfigSettings WHERE Name="imagePath";


imagePath is in this INSERT statement. I did not need to modify the default path in it which is why it does not show up in the diff, but the INSERT statement is still here :)

cmadjar · 2019-02-25T21:06:02Z

@driusan @PapillonMcGill Could you approve the PR if you are happy with the changes please? Thank you :)

driusan

Actually, could you change this to the major branch? It seems like it could introduce complications that should be caught in a LORIS testing round.. other than that it looks good.

removed mincPath and imagePath from get_file.php replaced all instances of imagePath and mincPath by dataDirBasepath in the code missed one instance of mincPath Melanie's feedback reintegrated imagePath to have one path for apache and one for the LORIS-MRI server if ever the paths are different forgot to add again the imagePath to the Config SQL default schema...

cmadjar · 2019-02-27T18:11:43Z

@driusan @PapillonMcGill I rebased the branch to major. Please re-review at your convenience :)

cmadjar mentioned this pull request Feb 8, 2019

Removed imagePath, mincPath and data from imaging_install.sh aces/Loris-MRI#399

Merged

1 task

cmadjar assigned driusan and PapillonMcGill Feb 8, 2019

PapillonMcGill previously requested changes Feb 11, 2019

View reviewed changes

SQL/0000-00-03-ConfigTables.sql Outdated Show resolved Hide resolved

SQL/New_patches/2019-02-08_cleanup_duplicated_data_path_in_Config.sql Outdated Show resolved Hide resolved

driusan reviewed Feb 11, 2019

View reviewed changes

cmadjar unassigned PapillonMcGill Feb 22, 2019

cmadjar commented Feb 25, 2019

View reviewed changes

driusan approved these changes Feb 25, 2019

View reviewed changes

driusan requested changes Feb 25, 2019

View reviewed changes

cmadjar force-pushed the cleanup_duplicated_data_config_paths branch from 413f2cd to def9351 Compare February 27, 2019 18:07

cmadjar changed the base branch from minor to major February 27, 2019 18:08

cmadjar added [branch] major and removed [branch] minor labels Feb 27, 2019

driusan approved these changes Feb 27, 2019

View reviewed changes

PapillonMcGill approved these changes Feb 28, 2019

View reviewed changes

driusan closed this Feb 28, 2019

driusan reopened this Feb 28, 2019

driusan merged commit 900ff1f into aces:major Mar 18, 2019

ridz1208 added this to the 21.0.0 milestone Mar 25, 2019

christinerogers mentioned this pull request Aug 1, 2019

Post-release 21 Documentation updates #4995

Closed

27 tasks

cmadjar deleted the cleanup_duplicated_data_config_paths branch September 24, 2021 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup duplicated data config paths #4314

Cleanup duplicated data config paths #4314

cmadjar commented Feb 8, 2019 •

edited

Loading

driusan Feb 11, 2019

cmadjar Feb 11, 2019

driusan Feb 13, 2019

cmadjar Feb 14, 2019

johnsaigle commented Feb 11, 2019

cmadjar commented Feb 11, 2019

cmadjar commented Feb 11, 2019

johnsaigle commented Feb 12, 2019 •

edited

Loading

driusan commented Feb 25, 2019

cmadjar commented Feb 25, 2019

driusan commented Feb 25, 2019

cmadjar left a comment

cmadjar Feb 25, 2019

cmadjar Feb 25, 2019

cmadjar commented Feb 25, 2019

driusan left a comment

cmadjar commented Feb 27, 2019

		@@ -187,9 +185,7 @@ INSERT INTO Config (ConfigID, Value) SELECT ID, "YMd" FROM ConfigSettings WHERE

		INSERT INTO Config (ConfigID, Value) SELECT ID, "/data/%PROJECTNAME%/data/" FROM ConfigSettings WHERE Name="imagePath";

Cleanup duplicated data config paths #4314

Cleanup duplicated data config paths #4314

Conversation

cmadjar commented Feb 8, 2019 • edited Loading

Brief summary of changes

This resolves issue...

To test this change...

Caveat for existing projects

driusan Feb 11, 2019

Choose a reason for hiding this comment

cmadjar Feb 11, 2019

Choose a reason for hiding this comment

driusan Feb 13, 2019

Choose a reason for hiding this comment

cmadjar Feb 14, 2019

Choose a reason for hiding this comment

johnsaigle commented Feb 11, 2019

cmadjar commented Feb 11, 2019

cmadjar commented Feb 11, 2019

johnsaigle commented Feb 12, 2019 • edited Loading

driusan commented Feb 25, 2019

cmadjar commented Feb 25, 2019

driusan commented Feb 25, 2019

cmadjar left a comment

Choose a reason for hiding this comment

cmadjar Feb 25, 2019

Choose a reason for hiding this comment

cmadjar Feb 25, 2019

Choose a reason for hiding this comment

cmadjar commented Feb 25, 2019

driusan left a comment

Choose a reason for hiding this comment

cmadjar commented Feb 27, 2019

cmadjar commented Feb 8, 2019 •

edited

Loading

johnsaigle commented Feb 12, 2019 •

edited

Loading