Support enabling IPM instance-wide #485

isc-tleavitt · 2024-05-22T20:09:09Z

This is done via new options in the existing zpm "enable" command that add package mappings.

We looked into using %ALL but ultimately that doesn't work, because namespace-specific mappings aren't allowed to override %ALL mappings (!?).

Fixes #470

Even %SYS, but not using the %ALL namespace because that ends up overriding namespace-specific mappings (gah).

isc-kiyer

@isc-tleavitt a few comments but looks good!

src/cls/IPM/Main.cls

gjsjohnmurray · 2024-07-14T04:03:20Z

namespace-specific mappings aren't allowed to override %ALL mappings (!?).

This always struck me as an unfortunate design choice. Too late to get it changed now, but maybe a second pseudo-namespace %DEFAULT could be added whose mappings defer to namespace specific ones.

src/cls/IPM/Main.cls

isc-tleavitt · 2024-07-25T17:15:08Z

namespace-specific mappings aren't allowed to override %ALL mappings (!?).

This always struck me as an unfortunate design choice. Too late to get it changed now, but maybe a second pseudo-namespace %DEFAULT could be added whose mappings defer to namespace specific ones.

@gjsjohnmurray thanks for chiming in! I like the idea and will float it, but we won't be able to make it a prerequisite for this work.

isc-shuliu · 2024-09-20T15:21:37Z

Not sure why the CIs are failing.

Tried to run the tests locally on 9b19985 it still fails although CIs passed in May ...

isc-shuliu · 2024-09-20T15:57:18Z

It appears the CI failure stems from this block

ipm/src/cls/IPM/Lifecycle/Base.cls

Line 1069 in bf8ed87

If '$Get(pParams("Verify","InCurrentNamespace"),0) {

isc-tleavitt · 2024-09-23T13:23:54Z

Next steps here for @isc-shuliu who's carrying this work forward:

Revert the changes to the MapOnly installer XData block and associated method, as they're causing the CI issues and are generally problematic
Reevaluate against criteria in Review and improve installation modes #470

isc-shuliu · 2024-09-25T21:42:13Z

CIs are failing specifically in I4H because migration was not successful (see log line 1362). This failure was introduced by changes in bf6e699 which attempts to perform data migration in all namespaces

Guess I shouldn't perform it in all namespaces according to this comment

ipm/src/cls/IPM/Utils/Migration.cls

Line 29 in 6a9dd01

// Intentionally runs for current namespace only

but I'm not sure why.

@isc-tleavitt Any insights?

isc-shuliu · 2024-09-26T15:05:17Z

This PR should now meet the requirement in #470 (comment) . Specifically,

It should be easy to say, after installing IPM, "I want this version of IPM to be used in all namespaces"

This can be done by mapping the current IPM globally

It should be easy to upgrade IPM instance-wide from any namespace to which the instance-wide installation is mapped (note: IPM installed version metadata needs to be tracked in the right DB!) - this might mean tracking the namespace through which IPM is managed, and only allowing upgrades through that namespace.

This is achieved by checking whether IPM in current NS is mapped if a user attempts to install/reinstall/uninstall IPM. If IPM is mapped from elsewhere, will abort and prompt the user to go the source namespace.

It should be easy, with or without an instance-wide installation of IPM, to install and run different IPM versions in different namespace.

This is achieved by adding the unmap command which allows a user to unmap IPM in give namespaces.

There should be a way to see which namespaces have IPM available, at which versions, and where it's mapped from in each namespace.

The zpm "version" command will show IPM versions from all namespaces, and if applicable, where it's mapped from.

Interactive use of the zpm command should direct the user properly if IPM is not available in the current context.

If IPM is not installed, will iterate through all namespaces and run zpm "version" in the first namespace found that has IPM installed.

After enabling of IPM in a namespace with 0.7.x (or earlier) installed, need to run the data migration process.

%IPM.Utils.Migration:MigrateZPMToIPM() will be called in each namespace

isc-tleavitt · 2024-09-26T17:10:19Z

CIs are failing specifically in I4H because migration was not successful (see log line 1362). This failure was introduced by changes in bf6e699 which attempts to perform data migration in all namespaces

Guess I shouldn't perform it in all namespaces according to this comment

ipm/src/cls/IPM/Utils/Migration.cls

Line 29 in 6a9dd01

// Intentionally runs for current namespace only

but I'm not sure why.
@isc-tleavitt Any insights?

The issue here is:

Migrating ZPM data to IPM... in namespace HSCUSTOM
Migrating repo registry... 
IPM data migration was not fully successful. You may be able to reinstall the impacted packages to correct the issues.
ERROR #5809: Object to Load not found, class '%IPM.Storage.Module', ID '1'

Migrating ZPM data to IPM... in namespace HSLIB
IPM data migration was not fully successful. You may be able to reinstall the impacted packages to correct the issues.
ERROR #5002: ObjectScript error: <PROTECT>SetMapSelectability+17^%SYSTEM.SQL.Util.1 ^rINDEXEXT("MS","%IPM_Storage","ModuleItem","IDKEY"),/usr/irissys/mgr/hslib/

Migrating ZPM data to IPM... in namespace HSSYS
Migrating repo registry... 
IPM data migration was not fully successful. You may be able to reinstall the impacted packages to correct the issues.
ERROR #5809: Object to Load not found, class '%IPM.Storage.Module', ID '1'

Migrating ZPM data to IPM... in namespace HSSYSLOCALTEMP
Migrating repo registry... 
IPM data migration was not fully successful. You may be able to reinstall the impacted packages to correct the issues.
ERROR #5809: Object to Load not found, class '%IPM.Storage.Module', ID '1'

Two classes of issues here:

HSLIB: here we can't migrate because the HSLIB database is read-only. In this case, we want to just skip. (Should be able to detect this.)
Others: likely related to presence of IPM in HSLIB itself but kind of hard to pinpoint with available data.

Ultimately, I think the issue is going to be that we should only try to migrate a namespace's data after it's mapped.

Data migration post-installation should migrate everywhere that is already mapped (mostly will be useful for future automatic data migrations); mapping should migrate the newly-mapped namespace. The automatic mapping post-installation on an environment with older ZPM will end up migrating each namespace as it's mapped. That covers all scenarios.

isc-tleavitt

See also #485 (comment) - soooo close.

src/cls/IPM/Main.cls

src/cls/IPM/Utils/Migration.cls

src/cls/IPM/Main.cls

isc-shuliu · 2024-09-27T16:16:53Z

@isc-tleavitt I updated the migration logic to run only in namespaces where %IPM is mapped from the current namespace. This resolved the problem for HSLIB but still doesn't successfully migrate HSCUSTOM, HSSYS, and HSSYSLOCALTEMP.

In I4H, HSCUSTOM, HSSYS, and HSSYSLOCALTEMP did not have %IPM mapping initially. Upgrading from %ZPM to %IPM automatically adds %IPM mapping for them. However, data migration still fails in these 3 namespaces.

isc-shuliu · 2024-09-27T17:41:15Z

@isc-tleavitt I think I figured out why:

In ##class(%IPM.Utils.Migration).MigrateOneRepo(), we should check for

Set newObj = ##class(%IPM.Repo.Definition).%OpenId(newId,,.sc)

instead of

Set newObj = ##class(%IPM.Storage.Module).%OpenId(newId,,.sc)

This is likely an error due to copy-paste from MigrateOnePackageToIPM()

isc-tleavitt · 2024-09-27T17:42:46Z

@isc-shuliu goooood catch! Thank you!

isc-tleavitt · 2024-10-04T14:43:39Z

I can't approve this (since it was originally my PR), but I think it's in a good state; would appreciate an eye on this from @isc-kiyer next week.

isc-kiyer

@isc-shuliu a few comments. Can maybe skip the one about changing the structure of the subscripted array to JSON since that would be more invasive (just the current structure is quite hard to follow).

isc-kiyer · 2024-09-27T18:25:40Z

preload/cls/IPM/Installer.cls

@@ -35,6 +35,7 @@ XData PM [ XMLNamespace = INSTALLER ]
    <Arg Value="${MODDIR}"/>
  </Invoke>
  <Invoke Class="IPM.Installer" Method="ZPMCompile" CheckStatus="true" />
+  <Invoke Class="IPM.Installer" Method="MapIfLegacy" CheckStatus="true" />


General question: how do you decide what should go into this installer block vs in the module.xml? Should each invoke here be there too?

My understanding is that <Invoke>s in module.xml should be a (not necessarily proper) subset of Installer.cls since a legacy user can always download a release artifact from GitHub or community PM registry.

Not sure if there are <Invoke>s that we only want present in Installer.cls but not module.xml.

@isc-tleavitt Thoughts?

In general, things only need to go into module.xml and the installer will run them. The issue here is in setting up mappings in advance of load when someone is already running legacy %ZPM and upgrades by loading/running an installer; that's why this particular thing needs to be here.

Wait - I'm wrong, we shouldn't need this because the above (line 34) call to ZPMLoad will end up running all the <Invoke>s.

I was confusing this with:
<Invoke Class="IPM.Installer" Method="Map" Phase="Reload" When="Before" />

@isc-shuliu @isc-tleavitt so should the installer invokes be removed in favor of the Invoke's in module.xml? The installer then essentially installs the module using itself which would run invokes from the module.xml?

src/cls/IPM/Main.cls

isc-kiyer · 2024-10-08T16:25:25Z

src/cls/IPM/Main.cls

+		Quit $$$OK
+	}
+
+	If tModuleName = $$$IPMModuleName {


I think this check should be moved to LoadNewModule instead to catch the case from the load command as well instead of just the install command. Before loading the file using $System.OBJ.Load(), can read the module name using xslts to confirm whether it can be loaded in the current namespace.

isc-kiyer · 2024-10-08T16:29:09Z

src/cls/IPM/Main.cls

-	// but need to decide what level of customization to provide
-	Quit ""
+	// TODO: Needs to support enabling IPM from here, also need to decide what level of customization to provide
+	Set tQuitOnError = $Get(pArgs(2))


Is this quit/halt on error stuff a general feature of ZLANG or something specific here? Could you add some more doc about this?

This is specific to the zpm command - should be covered in doc for %IPM.Main:Shell.

feat: support package mapping IPM everywhere

9b19985

Even %SYS, but not using the %ALL namespace because that ends up overriding namespace-specific mappings (gah).

isc-tleavitt changed the base branch from master to v1 May 22, 2024 20:09

isc-kiyer requested changes May 23, 2024

View reviewed changes

gjsjohnmurray reviewed Jul 14, 2024

View reviewed changes

src/cls/IPM/Main.cls Outdated Show resolved Hide resolved

gjsjohnmurray reviewed Jul 14, 2024

View reviewed changes

src/cls/IPM/Main.cls Outdated Show resolved Hide resolved

isc-tleavitt mentioned this pull request Sep 6, 2024

ZPM Inconsistencies intersystems/git-source-control#470

Closed

isc-shuliu self-assigned this Sep 20, 2024

isc-shuliu added 2 commits September 20, 2024 10:43

style: misc style change based on code review feedback

7a00208

docs(main): remove inaccurate description of mapping to %ALL

bf8ed87

isc-shuliu marked this pull request as ready for review September 20, 2024 14:56

isc-shuliu requested review from gjsjohnmurray and isc-kiyer September 20, 2024 14:56

isc-tleavitt requested review from isc-shuliu and removed request for gjsjohnmurray September 23, 2024 12:50

isc-shuliu added 7 commits September 23, 2024 10:48

fix: revert changes in MapOnly XData and Map Method

a9e8cbb

docs(enable): specify when -globally tag includes %SYS

0a6887e

enhance: show ns-specific IPM versions when calling zpm from ns w/o IPM

0ce3be4

fix: map IPM to all namespaces when upgrading from ZPM via registry

906423e

enhance: disallow management of IPM if mapped from another namespace

faf4ebd

fix: perform migration in all namespaces when upgrading from ZPM to IPM

bf6e699

feat: implement an "unmap" command to allow for per-NS upgrade of IPM

1de4bd0

fix: run migration in each namespace with its own try-catch

6db5287

isc-tleavitt commented Sep 26, 2024

View reviewed changes

src/cls/IPM/Main.cls Outdated Show resolved Hide resolved

src/cls/IPM/Utils/Migration.cls Show resolved Hide resolved

isc-tleavitt commented Sep 26, 2024

View reviewed changes

src/cls/IPM/Main.cls Show resolved Hide resolved

isc-shuliu added 4 commits September 27, 2024 10:19

doc: improve documentation of unmap

62d4a99

enhance: improve error handling based on pArgs, e.g. zpm "cmd":1:1

cc2e4f6

fix: only perform migration in NS with %IPM mapped from current NS

7b10d49

fix: run %IPM.Main in the correct NS

0778166

fix: should check repo (instead of package) works in MigrateOneRepo()

3a6a5ef

isc-shuliu approved these changes Oct 4, 2024

View reviewed changes

isc-kiyer requested changes Oct 7, 2024

View reviewed changes

isc-shuliu added 6 commits October 7, 2024 09:07

style: typo and style fixes

ed9589d

refactor: use %IPM.Main:IPMInstalled to retrieve IPM module ID

bf96419

enhance: remvoe redundant call to MapIfLegacy in IPM.Installer.cls

bad7332

style: more minor changes

d8ac1af

feat: use dynamic SQL to prevent unexpected problems

f7bb629

feat(enable): support -preview for all use cases

327eb17

isc-shuliu mentioned this pull request Oct 7, 2024

Use JSON instead of multidimensional array in AddIPMMappedNamespaces #580

Open

isc-shuliu requested review from isc-kiyer and isc-shuliu October 7, 2024 16:04

isc-kiyer reviewed Oct 8, 2024

View reviewed changes

isc-kiyer approved these changes Oct 8, 2024

View reviewed changes

isc-tleavitt merged commit 72fb28e into v1 Oct 10, 2024
15 checks passed

isc-tleavitt deleted the v1-enable-globally branch October 10, 2024 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support enabling IPM instance-wide #485

Support enabling IPM instance-wide #485

isc-tleavitt commented May 22, 2024 •

edited

Loading

isc-kiyer left a comment

gjsjohnmurray commented Jul 14, 2024

isc-tleavitt commented Jul 25, 2024

isc-shuliu commented Sep 20, 2024

isc-shuliu commented Sep 20, 2024

isc-tleavitt commented Sep 23, 2024

isc-shuliu commented Sep 25, 2024 •

edited

Loading

isc-shuliu commented Sep 26, 2024

isc-tleavitt commented Sep 26, 2024

isc-tleavitt left a comment

isc-shuliu commented Sep 27, 2024

isc-shuliu commented Sep 27, 2024

isc-tleavitt commented Sep 27, 2024

isc-tleavitt commented Oct 4, 2024

isc-kiyer left a comment

isc-kiyer Sep 27, 2024

isc-shuliu Oct 7, 2024

isc-tleavitt Oct 7, 2024

isc-tleavitt Oct 7, 2024

isc-tleavitt Oct 7, 2024

isc-kiyer Oct 8, 2024

isc-kiyer Oct 8, 2024

isc-kiyer Oct 8, 2024

isc-tleavitt Oct 8, 2024

Support enabling IPM instance-wide #485

Support enabling IPM instance-wide #485

Conversation

isc-tleavitt commented May 22, 2024 • edited Loading

isc-kiyer left a comment

Choose a reason for hiding this comment

gjsjohnmurray commented Jul 14, 2024

isc-tleavitt commented Jul 25, 2024

isc-shuliu commented Sep 20, 2024

isc-shuliu commented Sep 20, 2024

isc-tleavitt commented Sep 23, 2024

isc-shuliu commented Sep 25, 2024 • edited Loading

isc-shuliu commented Sep 26, 2024

isc-tleavitt commented Sep 26, 2024

isc-tleavitt left a comment

Choose a reason for hiding this comment

isc-shuliu commented Sep 27, 2024

isc-shuliu commented Sep 27, 2024

isc-tleavitt commented Sep 27, 2024

isc-tleavitt commented Oct 4, 2024

isc-kiyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isc-tleavitt commented May 22, 2024 •

edited

Loading

isc-shuliu commented Sep 25, 2024 •

edited

Loading