Terminate git pull on error

[isc-etamarch]

fixes #545

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 25.80%. Comparing base (f7666d8) to head (caaad30).
Report is 201 commits behind head on main.

Files with missing lines	Patch %	Lines
cls/SourceControl/Git/API.cls	0.00%	4 Missing ⚠️
cls/SourceControl/Git/Utils.cls	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #565      +/-   ##
==========================================
- Coverage   25.85%   25.80%   -0.06%     
==========================================
  Files          19       19              
  Lines        2738     2744       +6     
==========================================
  Hits          708      708              
- Misses       2030     2036       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[isc-etamarch]

Should also fix #342 if the pull is done using the terminateOnError argument

[isc-tleavitt]

Ultimately these are two distinct issues.

The terminate from #545 is a reference to $System.Process.Terminate, as used here (for example): https://github.com/intersystems/ipm/blob/master/src/%25ZPM/PackageManager.cls#L432 - this is for use from CI/CD scripts/pipelines/etc.

We don't want to use this in a CSP context, ever. Terminating the process with an OS-level error is too heavy-handed. From other contexts, there is a possible improvement to error reporting/logging, but it's possible that other changes over the summer have made the original requirement/issue moot. Try testing out with a bad SSH key on an SSH remote and see what pull does now.

[isc-etamarch]

Pravin tested and closed #342 because it no longer applies.

[isc-tleavitt]

Last thing - can you explicitly test that terminateOnError has the desired effect if an error occurs in the pull event handler? e.g., something fails to compile

[isc-tleavitt]

Last thing - can you explicitly test that terminateOnError has the desired effect if an error occurs in the pull event handler? e.g., something fails to compile

@isc-etamarch I can answer this through code inspection:
When SourceControl.Git.Utils:RunGitCommandWithInput runs the pull event handler, if an error occurs it puts the pull event handler output in errStream rather than outStream but doesn't return an error code.

We should have it return an error code in this case. Note, this is a higher risk change and we'll need to ensure other callers do the right thing. Alternatively we could do something hacky with a % variable we intentionally leak.

[isc-etamarch]

Last thing - can you explicitly test that terminateOnError has the desired effect if an error occurs in the pull event handler? e.g., something fails to compile

@isc-etamarch I can answer this through code inspection: When SourceControl.Git.Utils:RunGitCommandWithInput runs the pull event handler, if an error occurs it puts the pull event handler output in errStream rather than outStream but doesn't return an error code.

We should have it return an error code in this case. Note, this is a higher risk change and we'll need to ensure other callers do the right thing. Alternatively we could do something hacky with a % variable we intentionally leak.

SourceControl.Git.Utils:Pull checks the output of errStream for error / fatal. Should we also check for an ERROR, and would that grab the issue?

[isc-tleavitt]

SourceControl.Git.Utils:Pull checks the output of errStream for error / fatal. Should we also check for an ERROR, and would that grab the issue?

Yes, that would do it... as long as you're running in English.

[isc-tleavitt]

@isc-etamarch on this: just check for ERROR and call it a day.