Adding zpm "ci" command to install from a lock file - #1080
Conversation
isc-dchui
left a comment
isc-dchui
left a comment
There was a problem hiding this comment.
Good overall! Just a few small things. Also you'll want to add a description.
| set verbose = $get(commandInfo("data","Verbose")) | ||
| set log = ##class(%IPM.General.HistoryTemp).CleanInstallInit(moduleName) | ||
|
|
||
| // TODO: Add "path"? (see Update() for more info of calling install v load) |
There was a problem hiding this comment.
When is this todo being done or removed?
There was a problem hiding this comment.
It is being done. This was just a reminder to myself when putting up the first commit and PR for comments that I still need to account for that case.
…dule than install
| do ##class(%IPM.Main).Shell("repo -delete -name "_orasRepo) | ||
|
|
||
| // Install from Module G's lock file | ||
| do ..AssertInstallFromLockFileAsExpected(..#ModuleG) |
There was a problem hiding this comment.
One thing I noticed here is that the creation of the remote and oras repositories do not include username/password values as environment variables are not set. The repositories and test do still seem to pass but does anyone know if there's a way to set them in this test or should we add them as variables to the docker-compose.yml file?
There was a problem hiding this comment.
I believe the ORAS repo doesn't require username/password and the remote one is being configured with it (see line 142)
isc-dchui
left a comment
isc-dchui
left a comment
There was a problem hiding this comment.
A few things! Also still needs a description
| set commandInfo("parameters","module") = repository_"/"_depName | ||
| set commandInfo("parameters", "version") = version | ||
| set commandInfo("data", "LockFileInstallStarted") = 1 | ||
| set commandInfo("data","FromLockFile") = 1 |
There was a problem hiding this comment.
Do we need both "LockFileInstallStarted" and "FromLockFile"?
There was a problem hiding this comment.
We need different indicators to make sure we are only installing from a lock file of the base module (i.e. don't get to a dependency module and try installing its lock file too).
However, doing this in multiple parameters can be confusing. I have updated this to just have the "FromLockFile" parameter. When set to 1, then we do want to install from the lock file. This lock file method now sets it to 0, which indicates that we are already installing from a lock file so will ignore that in down the line modules.
| { | ||
| <Mapping xmlns="http://www.intersystems.com/jsonmapping"> | ||
| <Property Name="LockFileType" FieldName="type" /> | ||
| <Property Name="OverriddenSortOrder" FieldName="overriddenSortOrder" /> |
There was a problem hiding this comment.
Out of curiosity, what's the reason we're removing this?
| set modifiers("url") = lockFileValues.%Get("url") | ||
| set modifiers("namespace") = lockFileValues.%Get("orasNamespace") | ||
|
|
||
| // The following variables are set as system level variables for us to get |
There was a problem hiding this comment.
This should end up being documented somewhere else too. Definitely in the wiki and maybe even in the help text from zpm "help"
| do ##class(%IPM.Main).Shell("repo -delete -name "_orasRepo) | ||
|
|
||
| // Install from Module G's lock file | ||
| do ..AssertInstallFromLockFileAsExpected(..#ModuleG) |
There was a problem hiding this comment.
I believe the ORAS repo doesn't require username/password and the remote one is being configured with it (see line 142)
| @@ -310,13 +414,35 @@ ClassMethod AreLockFilesEqual( | |||
| expectedLockFilePath As %String) As %Boolean | |||
There was a problem hiding this comment.
Still should be %Boolean, but will update the catch block to $$$ThrowOnError instead of returning a status
| set sc = ##class(%IPM.Main).Shell("install " _ moduleName) | ||
| do $$$AssertStatusOK(sc, "Successfully installed " _ moduleName) | ||
|
|
||
| set packagePath = "/home/irisowner/zpm/tests/integration_tests/Test/PM/Integration/_data/lock-test/package/pack" |
There was a problem hiding this comment.
A bit brittle: this will break if running outside of containers
| set tSC = $$$ADDSC(tSC, tmpRepoMgr.CleanUp()) | ||
| $$$ThrowOnError(tSC) | ||
| if ($get(pCommandInfo("data", "FromLockFile"))) { | ||
| do ##class(%IPM.General.LockFile).InstallFromLockFile(tTargetDirectory, .tParams) |
There was a problem hiding this comment.
I believe the load path will not install the parent module since that's only in the "else" path and it slips past test10 accidentally.
There was a problem hiding this comment.
Good catch I missed that! Will make sure to add that checking in Test10 as well as the fix here
| do ..AssertInstallCreatesLockFileAsExpected(moduleName) | ||
|
|
||
| set sc = ##class(%IPM.Main).Shell("install " _ moduleName) | ||
| do $$$AssertStatusOK(sc, "Successfully installed " _ moduleName) |
There was a problem hiding this comment.
Should also verify that the modules and dependencies have been actually installed
There was a problem hiding this comment.
Out of curiosity, why not move this file into the .../IPM/General/LockFile directory?
There was a problem hiding this comment.
This was to be consistent with other directory paths under .../IPM/General/. LogManager, SemanticVersion, and SemanticVersionExpression all have class files directly under General/ with directories to hold other classes that are related
2 participants
No description provided.