gha-overhaul

[joshuafernandes]

PR description

• pre-review.yml -> ci.yml - this is the central file now
• release workflow updated. see the file for the flow now
• docker cleaned up. one job for build and test with goss, one for release which will sign and attest provenance with cosign. Tests are all done via goss now.
• renamed a few workflows to match intent in the workflow
• updated pyroscope version

Thanks for sending a pull request! Have you done the following?

• Checked out our contribution guidelines?
• Considered documentation and added the doc-change-required label to this PR if updates are required.
• Considered the changelog and included an update if required.
• For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

• spotless: ./gradlew spotlessApply
• unit tests: ./gradlew build
• acceptance tests: ./gradlew acceptanceTest
• integration tests: ./gradlew integrationTest
• reference tests: ./gradlew ethereum:referenceTests:referenceTests
• hive tests: Engine or other RPCs modified?

[macfarla]

verify-source-metadata dropped from the merge gate

The old unittests-passed gate required [compile, unitTests, verify-source-metadata]. The new one only requires [unitTests]. Supply-chain security checks (verifySourceArtifacts) no longer block PR merge. This feels like an unintentional regression — verify-source-metadata should remain in the required set.

[macfarla]

I don't think we want to include hidden files here?

[joshuafernandes]

verify-source-metadata dropped from the merge gate

The old unittests-passed gate required [compile, unitTests, verify-source-metadata]. The new one only requires [unitTests]. Supply-chain security checks (verifySourceArtifacts) no longer block PR merge. This feels like an unintentional regression — verify-source-metadata should remain in the required set.

yep, have added an allTestsPassed in ci.yml which catches that and the other test suites in one place

[joshuafernandes]

@siladu this will replace the containerVerify.sh Besu container started and entered main loop and relies on the internal healthcheck

[joshuafernandes]

this one replaces the containerVerify.sh version check - extract the version from the pod and match it to the tag. In ci.yml we skip this as we don;t care. In release.yml it sends the tag to compare against

[jflo]

good improvement!

[jflo]

have we tested that gradle does the right thing and avoids rebuilding the tarball when it is restored from the assemble-output?

Honestly, I'd be happy to remove distDockerCopy from gradle, and avoid depending on both it and java. It's just moving tarballs around and opening them.

[joshuafernandes]

Aye, should do, its what we do on teku & w3s.

Definitively in favour of less gradle mjolnir for everything ;) maybe a second round after this goes in? Fuse your changes in there too? In the very least this should simplify life a good deal

[siladu]

Still reviewing...let's test this in the sandbox before merging please 🙏

[siladu]

I fixed the linting and got the ci.yml green.
However, this PR regresses the overall CI time by double: from ~15mins to ~30mins
https://github.com/besu-eth/besu/actions/runs/28508814455

I appreciate it is easier to maintain this style of workflow but I think +15mins on every push to a PR is a case of negatives outweighing the benefits.

Before we commit to this workflow architecture can we address how we can might get the duration back down to ~15mins?

---

Some select items from a Codex review that I think deserve a comment or might be genuine bugs:

  - P1: CI no longer runs for merge queue events. The new workflow only has push and pull_request triggers in .github/workflows/ci.yml:3, and the test/gate jobs are additionally guarded with
    github.event_name == 'pull_request' around .github/workflows/ci.yml:151. Since the deleted workflows previously handled merge_group, required checks will not be reported for merge-queue
    entries. GitHub explicitly requires adding merge_group for required checks used with merge queues.

  - P1: Release Docker publish drops contents: read at the reusable-workflow call site. .github/workflows/release.yml:206 grants only id-token: write, so unspecified permissions become none,
    and reusable workflows cannot elevate them. The called workflow then tries actions/checkout in .github/workflows/docker-publish.yml:26, which needs repository contents. Add contents: read
    to the caller.

  - P1: Multi-arch Docker publish builds linux/amd64,linux/arm64 from an amd64 runner without explicit QEMU or a native arm builder in .github/workflows/docker-publish.yml:39. The Dockerfile
    runs apt-get during build in docker/Dockerfile:12, so the arm64 leg depends on emulation/native support. Docker’s multi-platform GitHub Actions example sets up QEMU before Buildx for this
    platform pair.

  - P2: The new release workflow no longer smoke-tests the generated tar/zip artifacts before drafting/publishing. draftRelease only needs build and dockerBuildTest in .github/workflows/
    release.yml:107; there is no Linux/Windows unpack-and-run check for besu --help / --version. A broken distribution archive could reach the draft/publish path.

[joshuafernandes]

I fixed the linting and got the ci.yml green. However, this PR regresses the overall CI time by double: from ~15mins to ~30mins https://github.com/besu-eth/besu/actions/runs/28508814455

Thankyou for that, appreciate the commits in there for spdx and code of conduct :)

I appreciate it is easier to maintain this style of workflow but I think +15mins on every push to a PR is a case of negatives outweighing the benefits.

Before we commit to this workflow architecture can we address how we can might get the duration back down to ~15mins?

This is quite misleading mate and also one of the big antipatterns this PR fixes. pre-review gives you the illusion of 15m builds, but the reality is you are waiting for all the other tests suites to finish running & then whatever going on in the docker workflows - you didn't know they were running because the workflow's name didn't suggest that, but they were. This surfaces everything so it's in one place, clear and easy to read + address things if they arise.
At best this adds 6m by putting the compile in line with tests. This helps with:

• guaranteeing determinism - we test the same bytes we ship. Parallel builds introduce the risk of non-deterministic output (timestamps, compiler nondeterminism, dependency resolution differences if a new version publishes mid-CI). Two builds from the same commit are usually identical, but not guaranteed.
• provenance - it's the chain of custody proof that the artifact in production is the one CI tested, not a rebuild.
• reproducibility of failures - when a test fails, you know exactly why. Lets be honest, how many times do we hit rebuild because a test is flaky? If we run builds again will they be 100% green or randomly break - do all N builds actually produce the same thing?
• cache coherence - with a single build artifact, all test jobs consume the same cached output. Parallel builds can thrash the cache or produce subtle cache-key collisions.

I don't have an objection to moving the compile back into each tests suite as opposed to passing it in, but you need to pick a point where you say this is the artifact we are shipping, whether that is before or after the tests is your call. @jflo @siladu @macfarla @usmansaleem @jframe what do you want to do here?

For comparison:

test	old	new
unit	13m (https://github.com/besu-eth/besu/actions/runs/28529685612)	12m (https://github.com/besu-eth/besu/actions/runs/28548008951)
ref	9m (https://github.com/besu-eth/besu/actions/runs/28529685612)	9m (https://github.com/besu-eth/besu/actions/runs/28548008951 )
acceptance	15m (https://github.com/besu-eth/besu/actions/runs/28529685612)	19m (https://github.com/besu-eth/besu/actions/runs/28548008951)
int	6m (https://github.com/besu-eth/besu/actions/runs/28529685612)	6m (https://github.com/besu-eth/besu/actions/runs/28548008951)

[joshuafernandes]

Some select items from a Codex review that I think deserve a comment or might be genuine bugs:

  - P1: CI no longer runs for merge queue events. The new workflow only has push and pull_request triggers in .github/workflows/ci.yml:3, and the test/gate jobs are additionally guarded with
    github.event_name == 'pull_request' around .github/workflows/ci.yml:151. Since the deleted workflows previously handled merge_group, required checks will not be reported for merge-queue
    entries. GitHub explicitly requires adding merge_group for required checks used with merge queues.

Another antipattern: merge_queue was tested at one point from what I understand, but not used. This PR calls that out and removes refs to it as well so its clear for users. This is discussed in the doc and with @macfarla already.

• P1: Release Docker publish drops contents: read at the reusable-workflow call site. .github/workflows/release.yml:206 grants only id-token: write, so unspecified permissions become none,
  and reusable workflows cannot elevate them. The called workflow then tries actions/checkout in .github/workflows/docker-publish.yml:26, which needs repository contents. Add contents: read
  to the caller.

Fixed: contents: read added to the release.yml

• P1: Multi-arch Docker publish builds linux/amd64,linux/arm64 from an amd64 runner without explicit QEMU or a native arm builder in .github/workflows/docker-publish.yml:39. The Dockerfile
  runs apt-get during build in docker/Dockerfile:12, so the arm64 leg depends on emulation/native support. Docker’s multi-platform GitHub Actions example sets up QEMU before Buildx for this
  platform pair.

On ubuntu:24.04 qemu already exists so this isn't needed, but have added it in for clarity regardless

• P2: The new release workflow no longer smoke-tests the generated tar/zip artifacts before drafting/publishing. draftRelease only needs build and dockerBuildTest in .github/workflows/
  release.yml:107; there is no Linux/Windows unpack-and-run check for besu --help / --version. A broken distribution archive could reach the draft/publish path.

Another antipattern: unzipping an artifact in linux/windows isn't a real test mate. Let's be honest, if the artifact zip/tar was unusable, it should have been picked up much earlier in the testing, not here. This is also why provenance helps.

[siladu]

pre-review gives you the illusion of 15m builds, but the reality is you are waiting for all the other tests suites to finish running & then whatever going on in the docker workflows

Here's an example PR with all the checks:

Preview is the longest check @ 15 mins , there's no waiting for extra tests or docker build+test after that, it's all in parallel https://github.com/besu-eth/besu/actions/runs/28560601462

I don't have an objection to moving the compile back into each tests suite as opposed to passing it in

The test suites are separate gradle commands, they don't rely on the artifact being generated. Code compile happens as part of the test command, even in this PR.

you need to pick a point where you say this is the artifact we are shipping, whether that is before or after the tests is your call

IIUC, release.yml rebuilds the artifact with a new gradle assemble command, it doesn't attempt to reuse what was built by ci.yml. I agree about the benefits of the provenance and testing what we ship (in the burn-in), but that is only relevant for release, whereas my regression comment is about the PR workflow, ci.yml

[siladu]

Made some changes to improve CI speed and clarity:

1. [-11 mins] - Remove dependency on compile job for test suites 796e77d
2. [-30 seconds] Split+parallelise checkLicense and spotless c4c03a2
3. [-3 mins???] Currently renamed test split workflows are causing a sub-optimal split - merging this PR should fix this for future PRs
4. Rename compile job to build since it does ./gradlew build 7a2616e

Total duration after this is merged should bring duration back down <= 15 mins: 29 mins - 11 mins - 3 mins - 30secs = ~14 mins 30 seconds

Will hold off on merging this until @joshuafernandes is back since once we merge, we'll need to test the release in the sandbox and potentially revert/forward-fix.