Fix deployment of changes that include modification of mappings

[isc-pbarton]

Fixes #864

Previously internal names for items in a deployment were all calculated at once before the import. Now they are lazily calculated when we import the individual item. Since we deploy changes to the configuration file first, this means changes to mappings are correctly taken into account.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 50.39%. Comparing base (18999e9) to head (a9910cb).

Files with missing lines	Patch %	Lines
cls/SourceControl/Git/Utils.cls	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #870      +/-   ##
==========================================
+ Coverage   49.77%   50.39%   +0.62%     
==========================================
  Files          24       24              
  Lines        3281     3272       -9     
==========================================
+ Hits         1633     1649      +16     
+ Misses       1648     1623      -25     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[raymond-rebbeck]

Unfortunately this seems to break items being deleted at the same time as their mappings in a pull.

I tried a pull where 1 LUT was added alongside a new trivial mapping (LUT, *, lut/) and it seemed fine, the LUT was imported. I then externally deleted the LUT and the mapping and pulled that, it could not determine the internal name of the LUT and did not delete it. (for the record the error was ERROR #5912: Page '' does not exist which is what happens when the type is unknown and Type() simply returns "csp", but that is coincidental)

It would seem that the old mappings may be required to correctly handle deletes. These did previously work but now the problem has perhaps just been shifted from adding to deleting.

The apparent complexity here in potentially needing both sets of mappings to handle deletes and adds in the same pull is part of what motivated #865 which attempts to extend the existing behaviour of CLS and INC files (possibly others too) in not needing mappings at all to determine their internal name, to more file types (HL7 and LUT).

[isc-pbarton]

You're right, it doesn't make sense to have that behavior inconsistent for CLS/INC and HL7/LUT. I will review your change #865.

I'd still like to merge this PR as well because it makes the behavior more logical for other item types. For example if I add a new CSP application.

[raymond-rebbeck]

Merging this PR will still break deletes though. #865 relies on being able to read the file which is only possible when it is being added, so it only helps when it is adding that is not working.

[isc-pbarton]

I might argue that the new behavior with deletes is better. Removing the mapping for LUT means I no longer want look-up tables to be controlled by Git at all. If I want Embedded Git to delete all the LUTs, I would keep the mapping and delete all the files. Does that sound reasonable?

[raymond-rebbeck]

I think the scenario of deleting an item and it's mapping with the intention of the item then still existing and subsequently being managed without Embedded Git is likely to be uncommon. There are other scenarios that I suspect are more common where the new deleting behaviour results in a regression that is (in my opinion) highly undesirable.

Switching branches is one such example. If one branch has a new mapping and corresponding item, then switching to another branch that does not have them currently results in the new item being deleted. However, with this PR the item will no longer be deleted which I think would almost always be undesirable and unexpected. I personally switch branches all of the time, such as when reviewing the work of others and would not like new items to stick around after I switch away from a branch just because the next branch does not (yet) have a new mapping from some new work.

Another example is where an item is deleted with the specific intent of it going away (i.e. the user actually really wants it deleted), then in a later separate commit (e.g. a week later) the corresponding mapping is then deleted too as it may be considered no longer in use by that point. If someone were to do a pull after all of this then it would not at that point in time be able to be distinguished from the situation of someone deleting the item and mapping at the same time with the intention of them remaining and being managed without Embedded Git. I think that the former case, deleting an item so that it's gone/deleted, is probably more likely. There's also the problem here that if another different team member did pulls more frequently, then they would have the item deleted, then in a later pull have the mapping removed (with nil effect otherwise as they have the item deleted already) so different team members could have different end results (one still has the item, the other does not) after pulls depending on when or how often they do them. I think this would be confusing for users and difficult to anticipate and work with, even being aware of it.

Irrespective of the above, this PR does break deleting in such a way that the pull handler output is inaccurate and difficult to make sense of. The following is an example with this PR code of switching from one branch (that has 1 new LUT and mapping) to a second branch that does not have them:

embedded-git-config.GSC has been imported from /git/embedded-git-config.json
ERROR #5912: Page '' does not exist
lut/ABC.lut was deleted.
Nothing to compile.

• The ERROR #5912: ... is confusing and is the result of ##class(SourceControl.Git.Utils).Type("") ("" due to blank internal name) resulting in "csp" and an attempt to delete the item as if it were a CSP file failing.
• It states the LUT was deleted, however it is in fact not (the error prior is the delete failing)
  • I would expect to see something more sensible, perhaps roughly the inverse of the current failure to add: X was not deleted...

In summary, I would very much prefer a fix for #864 that does not break the existing behaviour around deleting items. I concede that #865 is deficient (at least long term) in that it doesn't handle CSP applications at all (I don't currently use them with Git but imagine they're certainly not uncommon) or any other types at all beyond HL7/LUT. I'm not entirely certain what the best approach might be at this stage to have both adds and deletes working, though I wonder if a 2 pass pull event handler approach might work without requiring too much custom logic within pull handlers (and avoiding making them harder for anyone to implement their own), perhaps something like the following:

• Before calling the pull event handler, if the mapping file is being changed (it's in the list of modified files) then:
  • 1: Create a separate list of only the deleted files and run that through the pull event handler (first pass with old/current mappings)
    • Under the assumption that if we're deleting a file then we presumably have a mapping for it right now before the new ones are loaded
      • It's conceivable that we might not have a mapping until the new ones are loaded so perhaps evaluate the internal names of each deleted item first and only put known ones through in the first pass
  • 2: Put everything else (including mapping file) through the pull event handler (second pass with new mappings)

[isc-pbarton]

Fair enough, I appreciate the detailed argument and I certainly don't want to break a workflow you're already relying on. I'm going to close this out without merging.