fix(cli): require module name for uninstall #1018
Conversation
AshokThangavel
requested review from
isc-dchui,
isc-egabhart,
isc-eneil,
isc-jili,
isc-jlechtne,
isc-kiyer,
isc-pbarton and
isc-tleavitt
as code owners
isc-kiyer
left a comment
isc-kiyer
left a comment
There was a problem hiding this comment.
@AshokThangavel Looks good! Few minor comments. Also, this needs a changelog update
src/cls/IPM/Main.cls
Outdated
| set tModuleName = pCommandInfo("parameters","module") | ||
| set tModuleName = $get(pCommandInfo("parameters","module")) | ||
| if (tModuleName = "") { | ||
| $$$ThrowOnError($$$ERROR($$$GeneralError, "Module name is required for uninstall.")) |
There was a problem hiding this comment.
Nit: add to the message "when not using the -all flag"
There was a problem hiding this comment.
Yes, I’ve added a comment at the top of the code to address this.
There was a problem hiding this comment.
Adding it to the error message itself would also be handy (and I think what @isc-kiyer meant)
There was a problem hiding this comment.
Thank you. I've updated the error message to be more descriptive: A module name must be specified for uninstall unless the -all flag is used
| Method TestUninstallWithoutModuleName() | ||
| { | ||
| do $$$LogMessage("Run Uninstall command with out module name") | ||
| do ..RunCommand("unsintall") |
There was a problem hiding this comment.
This should check appropriate error was thrown
There was a problem hiding this comment.
Thank you! I've updated the code based on the comments
| do ..RunCommand("uninstall objectscript-math") | ||
| set exists = ##class(%IPM.Storage.Module).NameExists("objectscript-math") | ||
| do $$$AssertNotTrue(exists, "Module removed successfully.") | ||
| quit $$$OK |
There was a problem hiding this comment.
Nit: no need for a quit at the end
There was a problem hiding this comment.
Thank you! Removed the "quit" from the code
| do $$$LogMessage("Run Uninstall command with out module name") | ||
| do ..RunCommand("unsintall") | ||
| do ..RunCommand("install objectscript-math") | ||
| set exists = ##class(%IPM.Storage.Module).NameExists("objectscript-math") |
There was a problem hiding this comment.
Nit: fix spacing on this line
There was a problem hiding this comment.
Sure, I’ve fixed the spacing on that line.
src/cls/IPM/Main.cls
Outdated
| set tModuleName = pCommandInfo("parameters","module") | ||
| set tModuleName = $get(pCommandInfo("parameters","module")) | ||
| if (tModuleName = "") { | ||
| $$$ThrowOnError($$$ERROR($$$GeneralError, "Module name is required for uninstall.")) |
There was a problem hiding this comment.
Adding it to the error message itself would also be handy (and I think what @isc-kiyer meant)
| } | ||
|
|
||
| Method RunCommand(pCommand As %String) | ||
| // This method returns the %Status for checking the success or failure of executed commands. |
There was a problem hiding this comment.
Nit: method commands should use three slashes, not two
There was a problem hiding this comment.
I've updated to three slashes
|
|
||
| Method TestUninstallWithoutModuleName() | ||
| { | ||
| do $$$LogMessage("Run Uninstall command with out module name") |
There was a problem hiding this comment.
Nit typo: "without" and not "with out"
There was a problem hiding this comment.
Thank you! fixed the typo
3 participants
Description
This PR resolves an issue where the
uninstallcommand would proceed without a module name. In such cases, the command would default to the "implicit context" (the last active module), leading to accidental uninstallation of applications.fixes: #1017
Verification Results
Unit Test Passed:
TestUninstallWithoutModuleNameuninstall(no args) now returns aNotOKstatus.uninstall <name>still functions correctly.