Page MenuHomePhabricator

Wikibase GitHub workflow that extracts library commits is broken
Open, Needs TriagePublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:

Everything is red.

fatal: detected dubious ownership in repository at '/github/workspace'
To add an exception for this directory, call:

	git config --global --add safe.directory /github/workspace
fatal: Command '['git', 'show-ref']' returned non-zero exit status 128.

What should have happened instead?:
Everything is green and does the job it is supposed to do.

Background:

This GitHub workflow was created in context of T289040: Migrate DataModel, DataModelServices, DataModelSerialization, WBInternalSerialization to Wikibase.git. It is "extracting out" commits that relate to standalone packages in the Wikibase monorepo and pushing them to read-only repositories for that standalone component.

Source for this workflow: https://github.com/wikimedia/Wikibase/blob/master/.github/workflows/filterCommits.yml

It might have been broken by a change to Git itself: https://github.com/git/git/commit/8959555cee7ec045958f9b6dd62e541affb7e7d9

(Thank You to @Tarrow for helping me to figure out what might be going on here.)

Acceptance Criteria:

  • make that workflow work again
  • double check if any commits were missed while it was broken and if so, push them to the respective places
  • add some kind of email-notification or something, so that we notice when these jobs break again

Event Timeline

(Reminder: GitHub pull request approved but haven't merge.)

Change 894538 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Bump wmde/git-filter-repo-docker-action from v1 to v2

https://gerrit.wikimedia.org/r/894538

Change 894538 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Bump wmde/git-filter-repo-docker-action from v1 to v2

https://gerrit.wikimedia.org/r/894538

Change 894589 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Bump wmde/git-monorepo-splice-docker-action from v1 to v2

https://gerrit.wikimedia.org/r/894589

Change 894589 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Bump wmde/git-monorepo-splice-docker-action from v1 to v2

https://gerrit.wikimedia.org/r/894589

It’s finally working again.

image.png (254×292 px, 18 KB)

Should we keep the task open for the email notification AC, or close it?

double check if any commits were missed while it was broken and if so, push them to the respective places

Did we do this?

Should we keep the task open for the email notification AC, or close it?

I'll probably add a Wikibase Suite Team tag and leave it open, as I don't really feel like that this is my call to make. Maybe once they actually have developers, then they can decide what they want to happen here. But if anyone already has strong feelings on this, then I'm not blocking any resolution here either.

double check if any commits were missed while it was broken and if so, push them to the respective places

Did we do this?

I looked at the history of some of the repositories and saw new commits there; I don’t think a thorough check of all commits is needed: the action pushes the full history, not just the latest commit. On wmde/WikibaseDataModel a lot of commits got pushed, at least:

image.png (1×1 px, 452 KB)

Let’s tag the Suite then.

It’s finally working again.

image.png (254×292 px, 18 KB)

Perfect! Thanks.

double check if any commits were missed while it was broken and if so, push them to the respective places

Did we do this?

I looked at the history of some of the repositories and saw new commits there; I don’t think a thorough check of all commits is needed: the action pushes the full history, not just the latest commit. On wmde/WikibaseDataModel a lot of commits got pushed, at least:

image.png (1×1 px, 452 KB)

Let’s tag the Suite then.

Ah, I wasn't aware that this is how it works. Yeah, then I'll consider that AC met.

This broke again two months ago (failing action, gerrit commit).

Now the error is different - merge conflicts:

Auto-merging RELEASE-NOTES.md
CONFLICT (content): Merge conflict in RELEASE-NOTES.md
Auto-merging src/Entity/ItemId.php
CONFLICT (content): Merge conflict in src/Entity/ItemId.php
Auto-merging src/Entity/NumericPropertyId.php
CONFLICT (content): Merge conflict in src/Entity/NumericPropertyId.php
Auto-merging tests/unit/Entity/EntityIdTest.php
Auto-merging tests/unit/Entity/EntityIdValueTest.php
Auto-merging tests/unit/Entity/ItemIdTest.php
CONFLICT (content): Merge conflict in tests/unit/Entity/ItemIdTest.php
Auto-merging tests/unit/Entity/NumericPropertyIdTest.php
CONFLICT (content): Merge conflict in tests/unit/Entity/NumericPropertyIdTest.php
Rebasing (1/1)
error: could not apply 293be2693e... EntityId: Remove Serializable interface and methods
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 293be2693e... EntityId: Remove Serializable interface and methods

Not sure if we should open a new task for this or uncheck the ACs here. But I am noting that this task's AC for the email notification is still unfulfilled, which is why we did not notice this for two months. 🫤

I guess we can keep using this task, but if it was previously prioritized for just the last AC, we need to reprioritize it again. (But my impression is that it wasn’t really prioritized at all, at least not after the workflow was fixed last time.)

Comparing the history of lib/packages/wikibase/data-model in Wikibase and the history of wmde/WikibaseDataModel, I notice that both of them have “Remove obsolete comments and enable FunctionComment.WrongStyle sniff”, but after that, WikibaseDataModel jumps directly to “Drop reimplementations of is_iterable() & iterator_to_array()”, somehow skipping “EntityId: Remove Serializable interface and methods” and “EntityId: Assert valid serialization in unserialize()”. I don’t know how this could’ve happened (I don’t remember us force-pushing anything in Wikibase), but I think we might have to force-push WikibaseDataModel to resolve this 😖

Regarding "filtering commits" - not sure if you want to keep in this task

I've gone into a rabbit hole of sorts and didn't get a success yet:

  • git rebase commands that are the last of bringing changes from lib/packages/wikibase/data-mode to wmde/WikibaseDataModel fail to apply the commit 0d18f39caeac755b8509ef4951a2b329eba0c807 "EntityId: Remove Serializable interface and methods"
  • I seem to be able to reproduce this rebase failure and am moderately convinced it is because git rebase command somehow manages to miss the commit somewhere before the above one 4bcc371c75cd640f933fd2717b664b78c5a0d5b6 "Restore "EntityId: Hard-deprecate Serializable methods""
  • it seems to make sense that the Remove serializable.. commit fails to apply cleanly if the state of the branch misses changes from "Restore hard deprecate" as the latter touched many of the files the former, newer commit changes too
  • I managed to have a successful git rebase run if I manually (via interactive rebase) force git to include commit "restore hard depracate" before the other one
  • That said, success is not there as I have failed so far to:
    • fully understand why git rebase ignores one commit. It looks it might be something related to how rebase and git reverting commits interplay but no clean outcome to this thread yet
    • have a non-interactive git rebase command that could possibly be used in the CI to overcome the current challenge.