-
Notifications
You must be signed in to change notification settings - Fork 23
Make package
part of publish
#770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isc-shuliu just one small note
@@ -1296,6 +1296,12 @@ Method %Publish(ByRef pParams) As %Status | |||
{ | |||
Set tSC = $$$OK | |||
Try { | |||
// Perform the "Package" phase to prepare the module for publishing | |||
// ..Payload is populated by %Package. So need to make sure it's using the same lifecycle object. | |||
Set pParams("LifecycleObject") = $This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: for multiple phases of a single module, is the lifecycle class not provided in pParams? I understand it isn't for dependencies (and that makes sense) but across ExecutePhases(), it would be passed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lifecycle is passed in as a string (name of the class or a path on filesystem) in pParams("Lifecycle")
. Here we really need to use the same lifecycle object because %Package will write to the ..Payload
of the same object and %Publish read from it.
The ..Lifecycle
property is transient and created using %New()
in %IPM.Storage.Module:LifecycleGet()
. Somehow the r%Lifecycle
in LifeCycleGet
is null when called from %DispatchMethod()
, causing the getter method to create a new lifecycle object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isc-shuliu looks good! Just one question but approving
Catch up fix to #767; Implement #769
With this change,
%Package
is part of%Publish
and will be called duringpublish <module> -only
. This allows publishing the package without running%Reload
phase.