Add support for user-configurable ModuleRoot#1149
Conversation
- Implemented ModuleRoot setting in %IPM.Repo.UniversalSettings - Added rwx permission validation for custom directory paths - Updated module installation logic to prioritize configured ModuleRoot - Included unit tests to verify path redirection and permission checks
isc-dchui
requested review from
isc-cborbonm,
isc-egabhart,
isc-eneil,
isc-jili,
isc-jlechtne,
isc-kiyer,
isc-pbarton and
isc-tleavitt
as code owners
isc-kiyer
left a comment
isc-kiyer
left a comment
There was a problem hiding this comment.
@isc-dchui few small notes
| ClassMethod CheckDirPermission(directory As %String) | ||
| { | ||
| if '##class(%File).DirectoryExists(directory) { | ||
| $$$ThrowOnError(##class(%IPM.Utils.File).CreateDirectoryChain(directory)) |
There was a problem hiding this comment.
This is a side effect and should be documented and conveyed in the method name. The name sounds like its just checking. Not creating
| set sc = ..SetTestReportFormat(value) | ||
| } else { | ||
| set sc = ..SetValue($parameter(..%ClassName(1),key), value) | ||
| } elseif key = "ModuleRoot" { |
There was a problem hiding this comment.
Nit: use parameter since it exists
| $$$QuitOnError(##class(%IPM.Repo.UniversalSettings).SetValue("UseStandalonePip", "", 0)) | ||
| $$$QuitOnError(##class(%IPM.Repo.UniversalSettings).SetValue("SemVerPostRelease", 0, 0)) | ||
| $$$QuitOnError(##class(%IPM.Repo.UniversalSettings).SetValue("DefaultLogEntryLimit",20, 0)) | ||
| $$$QuitOnError(##class(%IPM.Repo.UniversalSettings).SetValue("ModuleRoot",##class(%SYSTEM.Util).DataDirectory() _ "ipm/", 0)) |
There was a problem hiding this comment.
Nit: can use parameter since it exists from UniversalSettings
| set customPath = ##class(%File).NormalizeDirectory("/tmp/ipm-test-root/") | ||
|
|
||
| // Ensure custom path exists and is writable before configuring it | ||
| do $$$AssertTrue(##class(%File).CreateDirectoryChain(customPath), "Created custom ModuleRoot directory.") |
There was a problem hiding this comment.
Should this be done in the test or do you want to test this dir gets created by IPM? Perhaps testing both cases where the dir already exists vs doesn't exist would be nice. Also I'm not sure how you would test this but it would be interesting to test a case where the instance doesn't have permission to the directory
There was a problem hiding this comment.
Split the test into the two cases.
For the permissions testing, I did some manual testing where I did chmod 555 /usr/irissys/ and then tried to set the module root to a non-existent directory:
zpm:USER>config set ModuleRoot /usr/irissys/ipm_temp
ERROR! Error creating directory chain /usr/irissys/ipm_temp/: <13> Permission denied
If ipm_temp already exists, then this error results:
zpm:USER>config set ModuleRoot /usr/irissys/ipm_temp
ERROR! No write permission on directory /usr/irissys/ipm_temp/
isc-dchui
force-pushed
the
feat/configurable-module-rrot
branch
from
e1d0370 to
ed91363
Compare
4 participants
Description
config set ModuleRoot /custom/path/Testing
Added both unit and integration tests
Checklist
mainbranch rebased or merged.zpm test -only) and integration tests (zpm verify -only) pass.