[FIX] git-aggregator issues in Debian bookworm

[PabloEForgeFlow]

Starting with git v2.27 pull no longer has a default strategy to reconcile divergent branches (see https://github.com/desktop/desktop/issues/14423#issuecomment-1105607066 and release notes) As a result, the following error is shown when no default strategy is configured and divergent branches are found:

hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint: 
hint:   git config pull.rebase false  # merge
hint:   git config pull.rebase true   # rebase
hint:   git config pull.ff only       # fast-forward only
hint: 
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.

The change was addressed in git-aggregator version 3.0.0 by https://github.com/acsone/git-aggregator/pull/64. Since both 16.0 and 17.0 Dockerfiles pin git-aggregator<3.0.0 and use Debian Bookworm (which has git>2.27), they are affected by the issue.

The proposed fix pins git-aggregator==4.0, which I've tested in 16.0 and seems to work just fine. If it is pinned to 3.0.0 for some reason I suppose we can find an alternative.

[ap-wtioit]

Pin was introduced in https://github.com/Tecnativa/doodba/pull/523 because of https://github.com/acsone/git-aggregator/issues/66 which was fixed in https://github.com/acsone/git-aggregator/issues/67

So the pin was originally reverted in https://github.com/Tecnativa/doodba/pull/524/

And then reintroduced in https://github.com/Tecnativa/doodba/commit/7b25d25d8e56beee2592b744facf9e518ffca2e6 because of https://github.com/acsone/git-aggregator/issues/68

Are you sure it works now? The last issue is still open.

BTW: we are running git-aggregator<3.0.0 just fine with git 2.37.7 (see https://github.com/Tecnativa/doodba/pull/535) on our systems.

[theangryangel]

FWIW I'm also seeing issues with 16.0 and 17.0 builds under GitHub actions - it almost seems like git-aggregator is ignoring the ssh/config file completely. Locally it seems to be working fine. I'm only about 5 minutes into looking into this in any detail. Any builds over the weekend have also failed, so I'm assuming it's related to this somehow.

[ap-wtioit]

Ah yes, we have a few more patches in 100-repos-aggregate that make it work.

# make sure odoo has a user.name configured, as merges would not succeed otherwise
# (even if GIT_AUTHOR_NAME and EMAIL are set and should be used, it seems gitaggregate is not passing them to git)
su --shell="$SHELL" odoo -c 'git config user.name 1>/dev/null || git config --global user.name "'"$GIT_AUTHOR_NAME"'"'

# enable merges in git pull, this seems necessary for git-aggregator to work properly with our rebased feature branches
su --shell="$SHELL" odoo -c 'git config pull.rebase 1>/dev/null || git config --global pull.rebase false "'"$GIT_AUTHOR_NAME"'"'

# configure main as the default branch to suppress warnings in gitaggregate
su --shell="$SHELL" odoo -c 'git config init.defaultBranch 1>/dev/null || git config --global init.defaultBranch main'

I wonder where those got lost on the way to upstream (this repo).

Edit: i added those in our repo on 23.01.2023 after https://github.com/Tecnativa/doodba/pull/535 was stopped for the official doodba. (I still think it's better to have one git version across multiple doodba versions as those patches are only for certain git versions)

[PabloEForgeFlow]

@ap-wtioit I haven't been able to reproduce https://github.com/acsone/git-aggregator/issues/68 in our 16.0 projects, maybe it was caused by the combination of git and git-aggregator versions?

I also considered using git config --global pull.rebase false, but if updating git-aggregator works we might just as well do that. This is our current workaround in 050-fix-git-reconcile:

#!/bin/bash
pip uninstall -y git-aggregator
pip install git-aggregator

@theangryangel We are having Host key verification failed errors on private repositories, but I assumed it was another unrelated issue. It is worth noting that aggregating locally does not trigger the error, so maybe it has to do with some build arguments?

[ap-wtioit]

@PabloEForgeFlow to me it looks like https://github.com/acsone/git-aggregator/issues/68 only happens in git version < 2.25.1 so Debian bookworm should not be affected by it and git-aggregator 4.0.0 should be fine for 16.0 and 17.0

[theangryangel]

@theangryangel We are having Host key verification failed errors on private repositories, but I assumed it was another unrelated issue. It is worth noting that aggregating locally does not trigger the error, so maybe it has to do with some build arguments?

Auto aggregate as the root user for me is failing because when these lines run https://github.com/Tecnativa/doodba/blob/master/16.0.Dockerfile#L227

The ~root/.ssh folder looks like this:

#9 0.449 doodba WARNING: ~/.ssh/
#9 0.454 total 20K
#9 0.454 drwxr-xr-x 2 root root 4.0K Nov 20 12:19 .
#9 0.454 drwx------ 1 root root 4.0K Nov 20 12:19 ..
#9 0.454 drwx------ 1 root odoo 4.0K Nov 20 09:10 ssh

Instead of this:

#9 0.412 drwx------ 1 root odoo 4.0K Nov 20 09:10 .
#9 0.412 drwxr-xr-x 1 root root 4.0K Nov 14 14:23 ..
#9 0.412 -rw------- 1 root odoo 1022 Nov 20 09:10 config
#9 0.412 -rw------- 1 root odoo 3.3K May 17  2023 glodouk_enterprise_id_rsa
#9 0.412 -rw------- 1 root odoo  734 May 17  2023 glodouk_enterprise_id_rsa.pub
#9 0.412 -rw------- 1 root odoo    0 Jul 24 13:43 id_rsa
#9 0.412 -rw------- 1 root odoo    0 Jul 24 13:43 id_rsa.pub
#9 0.412 -rw------- 1 root odoo 1.6K Nov 14 14:26 known_hosts

I've been trying to track down what in coreutils/bookworm seems to have changed, but as a temporary workaround I've set up this odoo/custom/build.d/099-fix-aggregate script to fix it up

#!/bin/bash
set -e

if [ -e ~root/.ssh/ssh/ ]; then
    log WARNING "Found nested SSH directory. Performing workaround for temporary fix for #582"

    unlink ~root/.ssh/ssh
    rm -rf ~root/.ssh/
    ln -sf /opt/odoo/custom/ssh ~root/.ssh

    sync
fi

I'll raise a fix for the Dockerfile's, but I'd really like to find some documentation, or something, somewhere about the change in ln behaviour :(

Edit: for visibility I've raised https://github.com/Tecnativa/doodba/pull/585

[pedrobaeza]

Uhm, there's an error in the CI. Can you check it?

[PabloEForgeFlow]

git-aggregator is not creating a merge commit for the repos.yaml in repos_merge despite 099-git_merge_no_ff. It appears the default pull behavior was changed to fast-forward in https://github.com/acsone/git-aggregator/pull/70 (4.0.0) and is overriding the .gitconfig setting.

I guess we need to either change the tests to ensure that an actual merge is performed or downgrade git-aggregator to 3.0.1.

[JordiBForgeFlow]

@PabloEForgeFlow Looks like it's better to fix the tests, right? Would you know how to do that?

[ap-wtioit]

@PabloEForgeFlow @JordiBForgeFlow the test was added by me to ensure merges are possible in doodba. (merge commits were not working at some time because the git user config was no longer taken from the environment variables).

we probably would need a repo with a reachable url and 2 always mergable branches that create a merge commit when merged. then we could get remove scaffoldings/repo_merge/custom/build.d/099-git_merge_no_ff. i'm thinking of a bare repo in custom/src prepared for this test.

what do you think about that? should i fix the test this way?

[PabloEForgeFlow]

Sounds good to me. However, if we only need to ensure merge commits are created perhaps using a local repository is simpler. The following produces a merge commit using git-aggregator 4.0:

#!/bin/bash
rm -rf /tmp/test-repo
mkdir /tmp/test-repo
cd /tmp/test-repo

git init
touch a.txt
git add a.txt
git commit -m a.txt
touch b.txt
git add b.txt
git commit -m b.txt

git checkout HEAD~1
git checkout -b divergent
touch c.txt
git add c.txt
git commit -m c.txt

cd -
cat <<- EOF > repos.yaml
./test-merge:
  remotes:
    local: /tmp/test-repo
  merges:
    - local master
    - local divergent
EOF
gitaggregate -c repos.yaml aggregate
git -C test-merge --no-pager log

[JordiBForgeFlow]

@ap-wtioit Can we do the changes suggested by @PabloEForgeFlow , or you do?

[ap-wtioit]

@JordiBForgeFlow your can do it, i would most likely only be able to do it next week, as the wkhtmltopdf issue took the priority 1 for us today.

[PabloEForgeFlow]

Not a pretty fix, but tests should be working now.

[PabloEForgeFlow]

@pedrobaeza Could you approve the workflow for the new commit? Thanks

[pedrobaeza]

It seems CI is broken with this change.

[PabloEForgeFlow]

My bad, I only tested test_repo_merge and forgot about test_repo_merge_aggregate_permissions. They both run fine locally now.