V1next history log #578

isc-shuliu · 2024-10-03T18:40:56Z

Implement #366
TODO List

isc-tleavitt · 2024-10-04T14:42:33Z

Re: "committed" - this would track, in non-dev mode, if installation was attempted but failed in some way - that is, it would be set to 0 in this case and 1 if installation succeeded. The reason for the failure (probably a %Status) would be worth recording as well for a failed installation.

isc-kiyer · 2024-10-07T12:46:21Z

Re: "committed" - this would track, in non-dev mode, if installation was attempted but failed in some way - that is, it would be set to 0 in this case and 1 if installation succeeded. The reason for the failure (probably a %Status) would be worth recording as well for a failed installation.

This comes into the overall lifecycle rework where we flag the phase in which a module reached during install and track the error that occurred as well. For modules in dev mode, can permit resuming install from the failed phase onwards and for non-dev mode, always start from scratch for the module.

isc-shuliu · 2024-10-08T16:20:55Z

Not sure why 2c240de doesn't work.

I have a local registry with modA and modB, where modB depends on modA. When installing modB, IPM will call %IPM.Utils.Module:LoadDependencies(), which uses %SYSTEM.WorkMgr to spawn a new process that runs ..LoadModuleReference().

The change in commit 2c240de is simple. We create a %Persistent history log object, save it in database, and pass the ID to the work unit. In the work unit, we save the success/failure and TimeEnd.

However, when calling %Save() from the spawned process, it raises the error Failed to acquire exclusive lock on instance of '%IPM.General.History. Why is there any lock involved?

isc-shuliu · 2024-10-08T21:40:04Z

It appears the lock is created because we're in a transaction. (Tried stepping through the code while monitoring management portal, lock disappears after manually running TCOMMIT)

isc-tleavitt

A few really minor comments, but I think this is great overall.

src/cls/IPM/General/History.cls

isc-tleavitt · 2024-10-11T17:16:42Z

src/cls/IPM/General/History.cls

+<Value>CommandString</Value>
+</Value>
+</Data>
+<DataLocation>^%IPM.General.HistoryD</DataLocation>


How is this mapped?

The data is mapped to each namespace where %IPM package is mapped. Do we want to separate them?

Nope, that'll be fine.

To clarify: there are no global mappings for the data but since it is a % global, will live in IRISSYS? I think its best to have this be a non-% global and can remove Namespace to scope history by namespace (like the rest of IPM).

isc-shuliu · 2024-10-15T14:45:44Z

@isc-tleavitt
Based on your feedback, I changed the time diff display to always show seconds and exposed API queries instead of API methods.

Currently, the table data is shared among all namespaces where %IPM is mapped. Do we want to separate them?

isc-tleavitt · 2024-10-21T17:11:40Z

I think this is good now, just need @isc-kiyer's review

isc-kiyer

@isc-shuliu This is a great and exhaustive feature! My comments and some questions are up

src/cls/IPM/General/History.cls

src/cls/IPM/Main.cls

isc-kiyer · 2024-10-24T17:52:29Z

src/cls/IPM/Main.cls

+	<parameter name="argument" required="false" description="Argument for `details` command" />
+	<example description="Show history of all packages in the current namespace in descending order">history find </example>
+	<example description="Show history of all packages in the current namespace where command starts with &quot;load&quot;">history find -DCommandString="load*"</example>
+	<example description="Show history of all packages in the current namespace where start time is later than 2000-01-01 00:00:00">history find -DTimeStart=">2000-01-01 00:00:00"</example>


Rather than using -D modifiers, may be nice to be able to add sub-modifiers and enum for parameters or modifiers. Then for history, can have enum for the action parameter and sub-modifiers tied to different parameter enums. Is a bit extensive and invasive but I think it will be helpful for many other commands.

Happy to talk through design of this

src/cls/IPM/Storage/QualifiedModuleInfo.cls

src/cls/IPM/Main.cls

isc-kiyer · 2024-10-25T14:51:45Z

src/cls/IPM/General/History.cls

+/// Get the history of all installations, uninstalls, and loads in given namespace
+/// The filter argument is a multidimensional array with structure 
+/// filter(columnName) = value
+/// Where value can optionally start with >, >=, <, <=, =, <> or contain * to indicate a wildcard


Rather than value starting with such things, perhaps make value a $ListBuild? The possible issue with it starting with that is that the property's value may actually be using one of these symbols.

That's a good idea, my concern is that It can be cumbersome for users to construct a command string that contains $ListBuild()s. They would need to do something like

zpm "history find -DTimeStart=" _ $lb(">", "2000-01-01 00:00:00")

And I can't think of a way for it to work in interactive mode:

USER>zpm ============================================================================= || Welcome to the Package Manager Shell (ZPM). Version: || || Enter q/quit to exit the shell. Enter ?/help to view available commands || || Current registry https://pm.community.intersystems.com || ============================================================================= zpm:USER>history find -DTimeStart=???

src/cls/IPM/General/History.cls

src/cls/IPM/Main.cls

isc-tleavitt · 2025-01-21T15:15:22Z

This should account for #704 (the pitfalls of being stuck here for a while, while we've been focused on more pressing things...)

isc-kiyer

@isc-shuliu another round of comments. Also, worthwhile adding unit tests since this is a brand new feature. Some test cases I can think about:

simple commands with no modifiers
commands with valid modifiers
commands with invalid modifiers
modules in dev and non-dev mode
testing all commands (load, install, uninstall)
filtering
altering number of history results to keep
searching per namespace vs globally
error handling for invalid filtering columns

isc-kiyer · 2025-01-22T11:40:28Z

src/cls/IPM/General/History.cls

+<Value>CommandString</Value>
+</Value>
+</Data>
+<DataLocation>^%IPM.General.HistoryD</DataLocation>


To clarify: there are no global mappings for the data but since it is a % global, will live in IRISSYS? I think its best to have this be a non-% global and can remove Namespace to scope history by namespace (like the rest of IPM).

isc-kiyer · 2025-01-22T11:42:10Z

src/cls/IPM/General/History.cls

+/// Action of this history record. Can be load, install, or uninstall
+Property Action As %String(VALUELIST = ",load,install,uninstall") [ Required ];
+
+/// Name of the package being loaded/installed/uninstall. This is not necessarily requried. E.g., when loading a nonexistent directory.


Nit: typo for required

isc-kiyer · 2025-01-22T11:43:29Z

src/cls/IPM/General/History.cls

+{
+
+/// Action of this history record. Can be load, install, or uninstall
+Property Action As %String(VALUELIST = ",load,install,uninstall") [ Required ];


@isc-shuliu What about module actions like compile, activate etc. Are these things we want to log? Or just in general, all commands from the zpm shell?
cc @isc-tleavitt

isc-kiyer · 2025-01-22T13:27:46Z

src/cls/IPM/General/History.cls

+Property NameSpace As %String [ InitialExpression = {$NAMESPACE}, Required ];
+
+/// User who initiated the action
+Property UserName As %String [ InitialExpression = {$USERNAME}, Required ];


Nit: Use %Library.Username

isc-kiyer · 2025-01-22T13:28:01Z

src/cls/IPM/General/History.cls

+Property Committed As %Boolean [ InitialExpression = 0, Required ];
+
+/// The command string that triggered the action
+Property CommandString As %String(MAXLEN = 8192) [ Required ];


Does command string need a maxlen?

isc-kiyer · 2025-01-22T13:37:12Z

src/cls/IPM/General/History.cls

+/// Where value can optionally start with >, >=, <, <=, =, <> or contain * to indicate a wildcard
+ClassMethod GetHistory(ByRef filter, ascend As %Boolean = 0, limit As %Integer = 0, namespace As %String) As %SQL.StatementResult
+{
+    Set limit = +limit // Coerce to number to prevent SQL injection


You can use ? like in where clauses as TOP ? to prevent SQL injection

isc-kiyer · 2025-01-22T13:40:42Z

src/cls/IPM/General/History.cls

+/// If detail is 1, will display full details each property on a separate line
+/// If detail is "header", will display the column names using the same style as the data
+/// Otherwise, will display on a single line, with some details omitted
+ClassMethod ShowColumns(rs As %SQL.StatementResult, detail As %Boolean = 0) As %Status [ Internal, Private ]


Rather than having detail be a fake boolean, perhaps have it be an enum? So valid options could be "header" and "full" perhaps?

isc-kiyer · 2025-01-22T13:47:29Z

src/cls/IPM/General/History.cls

+/// Output "clause" is the SQL WHERE clause as a string, container zero or more ? placeholders
+/// The "varargs" is an array of values to be substituted in the ? placeholders. 
+/// The "varargs" may already be populated with values, and new values will be appended to it.
+ClassMethod ConstructSQLWhere(filter As %String, namespace As %String, Output clause As %String, ByRef varargs As %String) As %Status [ Internal ]


Nit: here filter should be ByRef since it a multi-dim array

isc-kiyer · 2025-01-22T13:51:07Z

src/cls/IPM/Main.cls

@@ -1889,6 +1915,19 @@ ClassMethod LoadFromRepo(tDirectoryName, ByRef tParams) [ Internal ]
 }

 ClassMethod Load(ByRef pCommandInfo) [ Internal ]
+{
+	Set devMode = $Get(pCommandInfo("data", "DeveloperMode"), 1)


Should devMode default be 0? Without -dev flag, it is 0 right?

isc-kiyer · 2025-01-22T13:52:48Z

src/cls/IPM/Main.cls

+
+	#dim log As %IPM.General.History
+	Set log = ##class(%IPM.General.History).InstallInit(tModuleName)
+	Set devMode = $Get(tParams("DeveloperMode"), 0)


There is no tParams here. Did you mean to write pCommandInfo("data","DeveloperMode") ?

isc-shuliu added 7 commits September 19, 2024 09:37

feat(history): add class for load/install/uninstall history

145d0ac

Merge branch 'v1-next' into v1next-history-log

acdc0d9

refactor(history): make the class more user-friendly

97c1e56

feat: record install logs

04ffacf

feat: record uninstall logs

015c5dc

Merge branch 'v1-next' into v1next-history-log

2ad2312

feat: record load logs

314ae4e

isc-shuliu added 11 commits October 7, 2024 12:56

feat(history): add API & scaffold for querying history

12db14b

feat(history): allow dynamic filter of history via zpm command line

ee6b84e

feat(history): allow getting history globally

5deed56

enhance(history): make default action "find"

b534803

style: clean up

444eb68

refactor(history): always globally stash command string for logging

824aa9e

refactor(history): make package name optional, only check in finalize

dbda336

refactor(history): set source by repo name only

93c09b3

style: indent using tabs

f6c05dd

refactor(history): make TimeEnd optional, in case of a failure

b8d3106

wip(history): log subprocess dependency installs; not quite working yet

2c240de

isc-shuliu added 8 commits October 8, 2024 17:57

fix(history): move creation of history log object inside worker process

6929079

fix: remove unused storage definition

790049f

fix: typo fix

0856220

enhance: pretty print histories

de9e0e2

feat: allow displaying history table columns

c27633d

enhance: escape linefeed and carriage return in history table

0f2f7a4

fix(history): properly implement "committed"

fe9d62a

refactor: move escape sequence formatting to ShowColumns

208f86a

isc-shuliu added 3 commits October 10, 2024 13:39

feat: minor reordering of statements

7c14b69

style: minor changes

a6a7099

docs: add change log entry for history log

6164e6a

isc-tleavitt requested changes Oct 11, 2024

View reviewed changes

isc-shuliu added 2 commits October 11, 2024 14:25

enhance(history): display time diff always in seconds

c5f56f7

refactor: expose API queries instead of API methods

280d999

isc-shuliu requested a review from isc-tleavitt October 15, 2024 14:45

style: minor style change

8a20712

isc-tleavitt approved these changes Oct 21, 2024

View reviewed changes

Merge branch 'v1-next' into v1next-history-log

9fdc480

isc-kiyer requested a review from isc-eneil October 24, 2024 14:14

isc-kiyer requested changes Oct 25, 2024

View reviewed changes

isc-shuliu requested review from isc-jili and isc-jlechtne October 25, 2024 17:20

isc-shuliu and others added 12 commits October 30, 2024 12:00

docs: improve documentation and use custom datatypes

d10486b

style: misc changes

d1a606b

fix(history): package in log can be empty (e.g., load a nonexistent dir)

8d7e990

fix: defensive coding against nonexistent $$$ZPMCommandToLog

00aab23

fix(ci): exclude community containers with expired licences

d01cfc1

Merge branch 'v1-next' into v1next-history-log

4fa092c

refactor: use regex instead of pattern matching

1990cd0

fix: properly filter history by n.s. (%globals are available everywhere)

f300aee

fix: switch back to original NS when getting history globally

62f747d

feat: allow deleting by filter and prevent deleting all by default

c7811ec

fix: add guard for deleting all records in current ns

7bb8a68

fix: bump patch version of legacy zpm artifact

8c0d05c

isc-kiyer requested changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V1next history log #578

V1next history log #578

isc-shuliu commented Oct 3, 2024 •

edited

Loading

isc-tleavitt commented Oct 4, 2024 •

edited

Loading

isc-kiyer commented Oct 7, 2024

isc-shuliu commented Oct 8, 2024 •

edited

Loading

isc-shuliu commented Oct 8, 2024 •

edited

Loading

isc-tleavitt left a comment

isc-tleavitt Oct 11, 2024

isc-shuliu Oct 11, 2024

isc-tleavitt Oct 21, 2024

isc-kiyer Jan 22, 2025

isc-shuliu commented Oct 15, 2024

isc-tleavitt commented Oct 21, 2024

isc-kiyer left a comment

isc-kiyer Oct 24, 2024

isc-kiyer Oct 25, 2024

isc-shuliu Oct 30, 2024

isc-tleavitt commented Jan 21, 2025

isc-kiyer left a comment

isc-kiyer Jan 22, 2025

isc-kiyer Jan 22, 2025

isc-kiyer Jan 22, 2025

isc-kiyer Jan 22, 2025

isc-kiyer Jan 22, 2025

isc-kiyer Jan 22, 2025

isc-kiyer Jan 22, 2025

isc-kiyer Jan 22, 2025

isc-kiyer Jan 22, 2025

isc-kiyer Jan 22, 2025

V1next history log #578

Are you sure you want to change the base?

V1next history log #578

Conversation

isc-shuliu commented Oct 3, 2024 • edited Loading

isc-tleavitt commented Oct 4, 2024 • edited Loading

isc-kiyer commented Oct 7, 2024

isc-shuliu commented Oct 8, 2024 • edited Loading

isc-shuliu commented Oct 8, 2024 • edited Loading

isc-tleavitt 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

isc-shuliu commented Oct 15, 2024

isc-tleavitt commented Oct 21, 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

isc-tleavitt commented Jan 21, 2025

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

Choose a reason for hiding this comment

isc-shuliu commented Oct 3, 2024 •

edited

Loading

isc-tleavitt commented Oct 4, 2024 •

edited

Loading

isc-shuliu commented Oct 8, 2024 •

edited

Loading

isc-shuliu commented Oct 8, 2024 •

edited

Loading