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.
…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
| { | ||
| <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
There was a problem hiding this comment.
It will, but our tests have a number of places where /home/irisowner/zpm/tests/integration_tests/Test/PM/Integration/_data/... is hard coded in which this is consistent with. And the tests for now are always run in the container.
I think this should be kept as is for this issue and if we want to refactor, refactor all those references together in their own issue.
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.