-
Notifications
You must be signed in to change notification settings - Fork 8
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
Improvements to deployment of production items #695
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #695 +/- ##
==========================================
+ Coverage 40.89% 41.23% +0.34%
==========================================
Files 23 23
Lines 3157 3167 +10
==========================================
+ Hits 1291 1306 +15
+ Misses 1866 1861 -5 ☔ View full report in Codecov by Sentry. |
@@ -764,10 +765,6 @@ ClassMethod AddToServerSideSourceControl(InternalName As %String) As %Status | |||
continue | |||
} | |||
set @..#Storage@("items", item) = "" | |||
#dim sc as %Status = ..ImportItem(item, 1) |
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.
Review impact of this on setting timestamps
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.
I did some code review and testing - I don't think there's going to be any impact on timestamps. In the original diff that added this #125 the intent was to use this to deploy items added in the pull. Since then we've moved that logic back to the pull event handlers, so this is purely a duplicate. Timestamps will get updated because we call ImportItem() separately in every code path where we call AddToServerSideSourceControl().
@@ -24,14 +24,22 @@ Method OnPull() As %Status [ Abstract ] | |||
/// <var>pullEventClass</var>: if defined, override the configured pull event class | |||
ClassMethod ForModifications(ByRef files, pullEventClass As %String) As %Status |
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.
Tighten with try/catch and double check exception handling of callers (make sure they're using the returned %Status)
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.
Improved error handling and confirmed that callers are already checking status.
Resolves #690
Prior to this change we were calling ImportItem() twice for every item changed by a Git operation. Once through AddToServerSideSourceControl, and once through the pull event handler. This made every Git operation changing productions twice as long as it needed to be. This was only an issue for production items because other items have a timestamp from %RoutineMgr that lets us skip the import if the item is up to date in IRIS.
Also adds a separate table that logs deployments into IRIS through the pull event handler.