Make CI template compatible with private repositories

larowlan → credited drumm → .

The changes look good to me.

I went a bit through git history about CI_MERGE_REQUEST_PROJECT_URL in the two places where it was used:
- We were using origin before and that was changed here 📌 GitlabCI should fetch less from git Needs review , but I think it can be brought back.
- The other place where it was present was like that from the very beginning 📌 Add support for 'test only' changes to gitlab CI Needs review , and you (@larowlan) did that initial job, so you're familiar with the change already :-)

Marking RTBC.

Do we not need CI_MERGE_REQUEST_PROJECT_URL for MRs? What is "origin" in that case - the MR repo or the base repo? Sure I have seen the "test only" script failing somewhere because of an invalid git object ID, and now I wonder if that's because the MR repo was out of date compared to the base repo.

Ah, here it is: https://git.drupalcode.org/project/drupal/-/jobs/570956

A test-only job that looks like it succeeded (it should have failed) but because it ran no tests and resulted in an error:

From https://git.drupalcode.org/project/drupal
 * [new branch]        11.x       -> 11.x
ℹ️ Changes from 11.x
fatal: bad object 7ea369d937df18d7d4fb802f7249a7b7f7de4708
If this list contains more files than what you changed, then you need to rebase your branch.
1️⃣ Reverting non test changes
fatal: bad object 7ea369d937df18d7d4fb802f7249a7b7f7de4708
fatal: bad object 7ea369d937df18d7d4fb802f7249a7b7f7de4708
2️⃣ Running test changes for this branch
fatal: bad object 7ea369d937df18d7d4fb802f7249a7b7f7de4708

In case it it helpful, GitLab has some “magic” refs around MRs, https://docs.gitlab.com/ee/user/project/merge_requests/reviews/#checkout...

the merge request head ref (refs/merge-requests/:iid/head) that is available for each merge request. It allows checking out a merge request by using its ID instead of its branch.

CI_MERGE_REQUEST_REF_PATH contains the above value.

Regarding the invalid object error mentioned in #9: I am pretty sure that comes from these lines in test-only.sh:

export TARGET_BRANCH=${CI_MERGE_REQUEST_TARGET_BRANCH_NAME}${CI_COMMIT_BRANCH}
git fetch -vn --depth=50 "$CI_MERGE_REQUEST_PROJECT_URL" "+refs/heads/$TARGET_BRANCH:refs/heads/$TARGET_BRANCH"

echo "ℹ️ Changes from ${TARGET_BRANCH}"
git diff ${CI_MERGE_REQUEST_DIFF_BASE_SHA} --name-only
echo "If this list contains more files than what you changed, then you need to rebase your branch."

For a MR, $TARGET_BRANCH should be 11.x. Fetching with --depth 50 on 2024-01-04 would not reach 7ea369d937df18d7d4fb802f7249a7b7f7de4708.

Maybe, instead of fetching a branch, we should fetch a commit. Maybe the script should be less optimistic and fail when it tries to access a "bad object". (I vote yes and yes.) But both of those suggestions are out of scope for the current issue. I will add the tag for a follow-up.

Regarding Comment #10: the head of the MR is checked out. The hard part is getting the target branch.

From Comment #8:

Do we not need CI_MERGE_REQUEST_PROJECT_URL for MRs? What is "origin" in that case - the MR repo or the base repo?

That seems like the key question. I think "MR repo" will normally mean the issue fork. (For security issues, the MR repo and the base repo are both the private fork.) I am not sure which repo CI_MERGE_REQUEST_PROJECT_URL refers to, but I am pretty sure that origin refers to the MR repo/issue fork. That is where we clone the source branch for the MR.

I think that is what we want. For the test-only script, we want to find tests that the MR adds or changes. If the main branch of the base repo has commits that are not mirrored in the MR repo (issue fork or private fork) and those commits make test changes, then we do not want to look at those. The same idea applies to the spell-check job: we want to look at changes introduced by the current MR.

Short version: I agree with Comment #7: using origin is the right fix. Back to RTBC.

Not really possible to test this conclusively without merging it, so I've just done that. Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

It makes regression - spellcheck job (for ex) now comparing against issue's fork 11.x branch instead of project's (target repo) one

@andypost:

Why is that a bad thing? In Comment #12, I suggested that comparing against the 11.x branch in the issue fork is the right thing to do.

Sometimes issue forks has no 11.x branch yet or it's outdated so follow-up is 🐛 Spell-checking job fails with "Argument list too long" when too many files are changed Active

Maybe we should automatically update the target branch in the issue fork whenever

1. The target version of the issue changes.
2. The source branch is updated. (I think that means rebased on the target branch in the main repo.)

Probably that is not going to happen before we finish the transition from Drupal issues to GitLab.

Or maybe there is some way in the CI job to access HEAD...11.x (the common ancestor commit of the source and target branches).

@andypost: Maybe 🐛 The test-only script for GitLab CI does not reliably use the right commit Needs review is the right approach for both the test-only job and the spell-check job.

The issue fork’s 11.x branch should be completely irrelvant as its not the target branch, the canonical project’s 11.x branch is the target. Worst case, someone could push mismatched code to the fork’s 11.x designed to undermine the test results. Keeping the canonical branches automatically synced to every issue fork would be impractical since there are thousands of them.

Automatically closed - issue fixed for 2 weeks with no activity.

I chatted with @benjifisher about the followups. Two already exist related to the first item in #12. There are 🐛 The test-only script for GitLab CI does not reliably use the right commit Needs review and 🐛 Test-only job fails with "couldn't find remote ref refs/heads/11.x" when 11.x branch does not exist in fork Needs review . I have added a new issue for the second point. 🐛 Let the test-only script for GitLab CI fail when it tries to access a "bad object" right commit Needs review

And I forgot to remove the tag.