Support passing in `-env /path/to/env.json` at installation time. #704

isc-shuliu · 2025-01-14T03:17:05Z

Implement #632

Allow users to pass in one or more files at install time using syntax like install <pacakge> -env /path/to/json1;/path/to/json2
Added a new singleton class %IPM.General.InstallTimeConfig

Package developers can access this config using

 ##class(%IPM.General.InstallTimeConfig).GetArg("<their-package-name>", "key", "nested-key", "nested-nested-key")

The env.json should have package name as top-level keys, so that when installing dependencies, the env.json is also available.

{
  "my-package": { "key1": "value1" },
  "one-dependency": { "key2": "value2" },
  "another-dependency": { "key3": "value3" }
}

When multiple env.json files are provided, later ones override the earlier ones during merging
Special rules apply when merging json objects:
- undefined can be merged with any type
- Other mixed types (array vs object, array vs number, object vs string, etc) cannot be merged
- When 2 arrays are merged, treat them as special objects with numeric keys, meaning we won't extend array1 with array2

isc-kiyer · 2025-01-14T19:02:29Z

@isc-shuliu general q: thoughts on having the helper method for consumers return the full json for their package? Gives them more control then instead of extracting individual keys

isc-shuliu · 2025-01-14T19:53:39Z

@isc-kiyer
So ##class(%IPM.General.InstallTimeConfig).GetArg("<package-name>") will give them the full JSON for their package.

Do you think we should omit the <package-name> part, and maintain a context of the current package's name?

isc-kiyer

@isc-shuliu looks mostly great! Few small comments/questions

src/cls/IPM/General/InstallTimeConfig.cls

isc-kiyer · 2025-01-14T19:33:59Z

src/cls/IPM/General/InstallTimeConfig.cls

+    While iter.%GetNext(.key, .value, .type) {
+        If (type = "object") || (type = "array") {
+            If dest.%GetTypeOf(key) = "unassigned" {
+                Do dest.%Set(key, value.%New())


Why does value need a %New() here?

So my idea was that, whenever dest[key] is empty:

if src[key] is an array, we assign an empty JSON array [ ] and then keep recursing

if src[key] is an object, we assign an empty JSON object { } and then keep recursing

During the recursive calls, the newly created empty array [ ] or empty object { } will be populated.

Throughout this recursive function, I only call dest.%Set() with primitive types, newly created array [ ], or newly created object { }. This ensures dest[key] doesn't reference the same object as src[key]. In other words, I create a deep copy of src[key] and assign to dest[key].

If we use Do dest.%Set(key, value) where value is src[key] and modify src[key] later, dest will also change. This is an unexpected behavior.

isc-kiyer · 2025-01-14T19:35:30Z

src/cls/IPM/General/InstallTimeConfig.cls

+    Quit ..Config.%ToJSON()
+}
+
+Method SetJson(json As %String)


Would we ever be setting from anything other than a file?

Not at the moment. But just in case in the future we need the ability to inject config from another source (say network stream), I added this method. Shall I remove it?

isc-kiyer · 2025-01-14T19:39:51Z

src/cls/IPM/Lifecycle/Base.cls

+	Set paths = $ListFromString(paths, ";")
+	Set ptr = 0
+	While $ListNext(paths, ptr, file) {
+		$$$ThrowOnError(config.Load(file))


Should we also remove leading/trailing white space here before loading the file?

Whitespace is a valid character in file names on both Unix and Windows. But I think it's uncommon to have a trailing whitespace and totally illegal to have a leading one in an absolute path.
@isc-tleavitt Ideas on whether to strip the whitespace?

isc-kiyer · 2025-01-14T19:40:17Z

src/cls/IPM/Main.cls

@@ -272,9 +272,13 @@ load https://github.com/user/repository.git -branch feature-1
 <example description="Loads the module described in C:\module\root\path\module.xml but set pip timeout to 30 seconds">
 	load -extra-pip-flags "--timeout 30" C:\module\root\path\
 </example>
+<example description="Loads the module described in C:\module\root\path\module.xml using the C:\path\to\env.json as the install time configuration">
+load -env C:\path\to\env.json C:\module\root\path


This should have semi-colon between the paths

This is actually an env.json file + a package folder. I'll add a couple examples to emphasize the "semicolon-separated" syntax.

isc-kiyer · 2025-01-14T19:42:02Z

src/cls/IPM/Utils/LegacyCompat.cls

+    // Methods with [CodeMode = objectgenerator] can then use this to access configuration set in env.json
+    Set itconfig = ##class(%IPM.General.InstallTimeConfig).%Get()
+    If 'itconfig.IsEmpty() {
+        Set qSpec = qSpec _ "/multicompile=0"


How does multicompile affect regular use case outside of migration code?

isc-kiyer · 2025-01-14T19:59:43Z

src/cls/IPM/Main.cls



 <!-- Modifiers -->
+<modifier name="env" aliases="e" dataAlias="EnvFiles" value="true" description="Semicolon separated paths to the environment files in json format. See wiki for details." />


Could env be added as a possible modifier for module-action too? A use case could be using it in a custom phase for invokes

isc-kiyer · 2025-01-14T20:17:38Z

@isc-kiyer So ##class(%IPM.General.InstallTimeConfig).GetArg("<package-name>") will give them the full JSON for their package.

Do you think we should omit the <package-name> part, and maintain a context of the current package's name?

Correct. I think we should keep the package-name part since one module may want to access package-name for another dependency perhaps? I could go either way on that though. I think it may be more complicated to keep context than is worth the effort though, especially since its a singleton and modules could be getting installed in parallel.

isc-eneil · 2025-01-14T20:35:44Z

@isc-kiyer So ##class(%IPM.General.InstallTimeConfig).GetArg("<package-name>") will give them the full JSON for their package.
Do you think we should omit the <package-name> part, and maintain a context of the current package's name?

Correct. I think we should keep the package-name part since one module may want to access package-name for another dependency perhaps? I could go either way on that though. I think it may be more complicated to keep context than is worth the effort though, especially since its a singleton and modules could be getting installed in parallel.

I can see a use case for Sales when we'd like to install the same package in different namespaces with different configuration. It might be necessary to impose a standard for how to do this. Perhaps within the json object that is the value of the key there could be keys corresponding to the namespace in which configuration is applied?

src/cls/IPM/Main.cls

isc-shuliu added 8 commits January 13, 2025 22:13

feat: allow passing -env env.json for load/install/reinstall

3bd5f68

test: add test for -env /path/to/env.json

eefdcc7

chore: update change log about the env.json file

39c1be7

feat: support merging config files, instead of overwriting completely

d8d402c

test: add unit test cases for merging json

d24d8de

test: update test case to include multiple env.json files

3f8797d

fix: fix a bug that precludes nested arrays in env.json

ead3c78

chore: improve change log

0561797

isc-shuliu marked this pull request as ready for review January 14, 2025 15:08

isc-shuliu requested review from isc-kiyer, isc-tleavitt, isc-eneil, isc-jili and isc-jlechtne January 14, 2025 15:08

isc-kiyer requested changes Jan 14, 2025

View reviewed changes

isc-eneil approved these changes Jan 14, 2025

View reviewed changes

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

isc-shuliu added 3 commits January 14, 2025 22:43

fix: change based on code review

41d86a9

feat: support passing in env files for module-actions

34472a3

fix: add an Any datatype for compat. with earlier version of IRIS

4e6c48a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support passing in `-env /path/to/env.json` at installation time. #704

Support passing in `-env /path/to/env.json` at installation time. #704

isc-shuliu commented Jan 14, 2025 •

edited

Loading

isc-kiyer commented Jan 14, 2025

isc-shuliu commented Jan 14, 2025 •

edited

Loading

isc-kiyer left a comment

isc-kiyer Jan 14, 2025

isc-shuliu Jan 15, 2025 •

edited

Loading

isc-kiyer Jan 14, 2025

isc-shuliu Jan 15, 2025

isc-kiyer Jan 14, 2025

isc-shuliu Jan 15, 2025

isc-kiyer Jan 14, 2025

isc-shuliu Jan 15, 2025

isc-kiyer Jan 14, 2025

isc-kiyer Jan 14, 2025

isc-kiyer commented Jan 14, 2025

isc-eneil commented Jan 14, 2025



		<!-- Modifiers -->
		<modifier name="env" aliases="e" dataAlias="EnvFiles" value="true" description="Semicolon separated paths to the environment files in json format. See wiki for details." />

Support passing in -env /path/to/env.json at installation time. #704

Are you sure you want to change the base?

Support passing in -env /path/to/env.json at installation time. #704

Conversation

isc-shuliu commented Jan 14, 2025 • edited Loading

isc-kiyer commented Jan 14, 2025

isc-shuliu commented Jan 14, 2025 • edited Loading

isc-kiyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isc-shuliu Jan 15, 2025 • edited Loading

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-kiyer commented Jan 14, 2025

isc-eneil commented Jan 14, 2025

Support passing in `-env /path/to/env.json` at installation time. #704

Support passing in `-env /path/to/env.json` at installation time. #704

isc-shuliu commented Jan 14, 2025 •

edited

Loading

isc-shuliu commented Jan 14, 2025 •

edited

Loading

isc-shuliu Jan 15, 2025 •

edited

Loading