Skip to content

[infra] Add a dist and dependency review guide - #1022

Merged
wenjin272 merged 2 commits into
apache:mainfrom
weiqingy:894-2b-dist-guide
Aug 21, 2026
Merged

wenjin272 merged 2 commits into
apache:mainfrom
weiqingy:894-2b-dist-guide

Conversation

@weiqingy

Copy link
Copy Markdown
Collaborator

Linked issue: #894

Purpose of change

code_review.md's Change-Type Review Guides table had one row left reading planned. A reviewer facing a change to what the project ships fell back to the general passes, which ask the right question without saying what breaks in this area.

This adds review-guides/dist-dependency.md and links that row, completing the set of four guides.

The guide is organised around the failures the build does not report, rather than around the three sub-areas the table's Focus cell names, because the mechanisms cut across them. What it covers:

  • A module missing from dist/pom.xml can compile, test, and release green while shipping none of its classes. The shipped set is a hand-written list of dependencies rather than anything derived from the reactor.
  • Nothing in the build validates a NOTICE, so a transitive addition can change what is bundled without appearing in the diff and without any check noticing.
  • Nothing is relocated, so a bundled dependency's version can be settled by Maven mediation rather than declared, and the version one integration pulls in becomes the version the others get.
  • Moving a dependency into or out of provided or test changes what ships, while moving between compile and runtime does not.
  • An exclude inside a shade filter whose artifact pattern matches everything applies to every artifact being shaded.
  • The licence check keys on the Apache License URL, so a header that keeps that URL passes with prose lines dropped or altered.

The dist registration sub-area has no example row. No reviewer has raised a missing dist registration on any PR in this repository, merged or open, across the full inline-comment history. Rather than stretch a citation, that sub-area is covered by a checklist bullet and cited to nothing.

Tests

No code changes, so verification was of the guide's claims and its published commands.

Every command in the Validation block was run as written, at two different substitutions of the <version> placeholder. ./tools/check-license.sh exits 0, and the new file needs no licence header because tools/.rat-excludes already covers review-guides/* — confirmed with a control file rather than by reading the glob.

The factual claims were verified against built artifacts rather than against the poms. The scope claim was measured across 177 artifacts on a built fat jar: compile and runtime dependencies are all present, provided and test dependencies all absent. The licence-header behaviour was established by running Apache RAT against deliberately degraded headers. The relative link was resolved by hand, since no markdown link checker runs anywhere in the repository.

API

No. Documentation only.

Documentation

  • doc-needed
  • doc-not-needed
  • doc-included

Was this patch authored or co-authored using generative AI tooling?

  • Yes
  • No

Generated-by: Claude Code 2.1.233 (Claude Opus 5)

Fills the last planned row of the Change-Type Review Guides table in
code_review.md with a focused guide for changes to what the project
ships or bundles.

The guide covers the failure modes the build does not report: a module
absent from the hand-written list in dist/pom.xml, a NOTICE that no
check validates, a bundled version chosen by dependency mediation, a
shade filter that matches more than it was written for, and a licence
header that passes a check keyed on the Apache License URL.

Generated-by: Claude Code 2.1.233 (Claude Opus 5)
@github-actions github-actions Bot added doc-included Your PR already contains the necessary documentation updates. fixVersion/0.4.0 priority/major Default priority of the PR or issue. labels Aug 17, 2026
@wenjin272
wenjin272 merged commit 501686b into apache:main Aug 21, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-included Your PR already contains the necessary documentation updates. fixVersion/0.4.0 priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants