-
Notifications
You must be signed in to change notification settings - Fork 24
Fix v0.10.5 release blockers #1016
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
base: main
Are you sure you want to change the base?
Conversation
isc-kiyer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isc-dchui few minor comments. Looks good!
| set tParams("DeveloperMode") = pOverrideDeveloperMode | ||
| } | ||
|
|
||
| if $get(pParams("Update")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior for install and update should be identical. Also, we need to ensure IgnoreInstalled and UpdateSnapshots are also not passed to dependencies (in addition to DeveloperMode).
It would be good to have a complex dependency test case which currently would result in infinitely installing dependencies if IgnoreInstalled is passed as 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix actually realigns Install and Update to both avoid propagating dev mode to dependencies. In the code flow, pParams("Update") is set during the Update command, and pParams("Install") during the Install command if the Update flag is not set.
The test for install is Test.PM.Integration.LoadModuleWithDeps.TestParamPropagation and the one for update is Test.PM.Integration.Update.TestDevModePropagation
I believe @isc-jili is tackling the IgnoreInstalled and UpdateSnapshots issues
|
|
||
| /// This method is called directly in LoadNewModule() instead of with an <Invoke> in the module.xml | ||
| /// because the old mappings break load/install before any <Invokes> can be run. | ||
| ClassMethod CleanupOldMappings() As %Status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this not be called from the Invoke() in IPM.Installer above in the XData block instead? It should also probably query all namespaces on the instance to fix these mappings in namespaces that map IPM instead of installing it in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that and it didn't work. For details, I set up the old mappings using
do ##class(%IPM.Utils.Module).AddGlobalMapping($namespace,"oddStudioDocument:(BEGIN):(""%ZP"")","IRISLIB")
do ##class(%IPM.Utils.Module).AddGlobalMapping($namespace,"oddStudioDocument:(""%ZP""):(""A"")","IRISSYS")
do ##class(%IPM.Utils.Module).AddGlobalMapping($namespace,"oddStudioDocument:(""Ens""):(""Ent"")","ENSLIB")
then called load /home/irisowner/zpm to install IPM again, which resulted in this error:
ERROR! User defined document 'ZPM.ZPM' not supported. No user defined document class in this namespace.
> ERROR #6315: Errors reporting importing XML subelement in file '/home/irisowner/zpm/module.xml' at line '3' offset '28', skipping this item.
It seems the problem is that the mappings break IPM before any of the lifecycle stages can happen, which is when an <Invoke> would occur.
Good call on checking the other namespaces though, I've added that change.
546fa99 to
f076a58
Compare
Fixes #999
Fixes #1000
Fixes #1007
Fixes #1015