Controlled Package reassignment on Pull (potentially a lot of refactoring)

[sbcgua]

Let's start from far. It would be nice if AG did fully automated and one-click code deployment. Unfortunately for large products it is not so. Usually a relatively large change (including DDIC changes, code changes, package reassignments, adding/deletion of classes) supposes clicking Pull multiple times (I remember like 6-7 times ...), suppressing activation errors and etc. I think that fully automated deployment is kind of a dream right now or at least not achievable in one step for sure. So I'd focus on a closer aim - make the deployment more controllable to be able to do portions of it in stages.

The #6090 was a good step in that direction. Another step that would be nice is separate package reassignments. Upon a complex update import different local/remote packages makes a huge mess as there are no proper diffs. I'd like to have a button (in a Pull dialog) to apply this change only and then to be able to observe the changes in aligned way. Now this is not possible. It seemed like an easy task but appeared very far from that. The responsible code is very intertwined, structures are indirect, some names are very generic to understand what was intended. I can probably try to go forward but I'm tentative to start it, because it will probably involve a lot of changes, probably structure changes, maybe class changes ... so a lot of refactoring in several iterations, and if I start it I'd like to have support: openness for such changes, some advises (as I don't know this part of the code well) and some testing and of course constructive reviews on the way :)

A bit more on code.

Playground structure.

$DESERTEST (PKG)
  $DESERTEST_1 (PKG)
    ZDESERTEST1 (PROG)
  $DESERTEST_2 (PKG)

The case: ZDESERTEST1 prog is changed and reassigned to TEST2 package.

Now what happens now if I pull this. AG shows 2 dialogs, which is already confusing!


But although somewhat clunky, intuitively I expect this work from the box. Just select "Change assignment" in the first dialog and skip the second ! Bingo ! But no ... it just does nothing ...

... and by the way the deserialization list comes empty to ZCL_ABAPGIT_OBJECTS=>DESERIALIZE

Let's debug

The file status returns more or less understandable results:

Though it is confusing why abap files do not have PACKMOVE flag. Though nothing critical at this stage

Then we come to object_checks->DESERIALIZE_CHECKS and this start to be very confusing. First there is warning_overwrite_find which first produces almost logical data set (let alone the "delete and recreate" + "Add local" in one place as well as no mention of TEST2 package)

But the last statements delete comparing OBJ_TYPE/NAME reduces it one line

Which is even OK for my goal :) If it worked. But certainly confusing from the code logic perspective.

then another call named warning_package_find produces this

And as I mentioned if I select just the "Change assignment" option it does nothing. But what happen if I choose both ? Well, it works :) Good for the user ... in this simple case. But how it looks from the code perspective ?

The ZCL_ABAPGIT_OBJECTS=>DESERIALIZE receives one line (just one, not 2 as I could expect from the dialogs)

The line has local-delete status, and no packmove. Sadly, because a very promising change_package_assignments call a bit later is just skipped. And the program is updated and reassigned by the dedicated object class. Which, I confess, the expected end result ... but with not control and with some dark magic in between. :))

So far this was y journey I this subject.

Where I would probably start. probably I'd reconsider how those pieces of code exchange the information. E.g. most of the code above uses zif_abapgit_definitions=>ty_results_tt type, which (let alone a very unclear name) is very generic, so the code parts in between must do a lot of work to infere all kinds of things. I'd try to make it more readable and direct. Also probably I'd look into how classes call each other, they are very coupled now, I'd probably think how to transfer data with params instead of objects. But just thoughts. No clear plan. Just directions. The problem appeared a lot more complex than I originally expected.

What do you think ?

[larshp]

yea, should probably refactor it

tho, suggest first get rid of the popup, and make a page, so all information can be shown in a structured way

[mbtools]

whew, that's a lot. first, I'm much more optimistic when it comes to "seamless deployment". I fixed dozens of bugs in this area and in general it works very well. (see CI results). Installation of quite large repos for example from https://github.com/stockbal are ok, too.

Still there are known issues:

• Dependencies between objects of the same type (like SMIM, IOBJ, DDLS)
• Creating a new namespace (NSPC: Automatic creation of namespaces #6210)

Most can be fixed and solutions are proposed. Cyclical DDLS are probably an exception since SAP doesn't handle these well either.

Changing package assignment and object at the same time is tricky, indeed, and could be improved. However, this does not happen too often. If big package reassignments are necessary, it's recommended to do it in a separate PR/transport anyway, without changing the object (note: package interfaces!).

For me a clear goal should be to put zcl_abapgit_objects behind an interface:

• Allow AG v1 and v2 implementations
• Constructor with io_dot
• Input/output parameters only as tables: items for local, files for remote
• No repo and UI logic inside (no io_repo, no popups)
• Items = tadir (cases like filenames for SICF or skipping objects for SADL need to be handled by the object type)
• Allow object class to specify dependencies (to another object type or to other object of the same type)
• Separate read/write of files from import/export of the object

Status calc and deserialize check should be removed from zcl_abapgit_objects. These are things that happen before i.e. like "staging from remote to local" followed by "commit and activate". As Lars wrote, a page makes sense fore this. Nevertheless, it needs to be possible to automated as it is now (ie. "yes" to all checks, for example for CI and background jobs).

Also, the "table change will cause data loss" check should be done during this check and not during deserializing.

Anyway, step-by-step. Let's start with agreeing on an interface.