[
https://issues.apache.org/jira/browse/IGNITE-29261?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Aleksandr Chesnokov updated IGNITE-29261:
-----------------------------------------
Description: Let's remove generated wire format JSON files from Git so
contributors no longer need to update and commit them. As part of the normal PR
checks, CI should generate the descriptions, including message IDs, for the
target branch and the result of a test merge of the PR, then show their diff in
GitHub Actions Job Summary. This should work for any target branch that has the
required generators. Format changes should produce a warning for review, while
build or comparison errors should fail the check. Updating the compatibility
label through workflow_run is a separate task. (was: Review specification, 12
November 2026.
* Ticket: https://issues.apache.org/jira/browse/IGNITE-29261
* PR: https://github.com/apache/ignite/pull/13802
* Reviewed branch: {{ignite-29261}}, head
{{47ffdfebf2b5da486a8eb6d372ca798d5da59deb}}.
* Comparison base for this review: {{master}},
{{b0c397908d0b336459620bb1865f790597aae800}}.
This document records the agreed scope and later decisions for this ticket. It
gives a reviewer the context without the earlier chat. The follow-up work below
is a separate task, not a requirement for this PR.
h2. Problem and goal
Ignite generates JSON descriptions of message fields, IDTO classes, enums and
message IDs. Today, contributors must update and commit these files. A
contributor may only learn about this requirement after CI runs near the end of
review.
We want CI to generate the descriptions and show the changes before merge,
without an extra JSON commit. A JSON diff means that the wire format or
registrations need review. It does not prove that compatibility is broken.
Compatibility tests and manual review remain necessary.
h2. Scope of this ticket
# Remove generated JSON files from {{modules/wire-compatibility-check/wire/}}
in Git, including {{ids.json}}.
# Generate and collect the files under
{{modules/wire-compatibility-check/target/classes/META-INF/ignite-wire/}}.
# Remove the CI check that requires generated JSON changes to be committed.
# Compare descriptions from two builds during the normal PR checks and publish
their diff in GitHub Actions Job Summary.
# Keep the existing checks for missing message descriptions and descriptions
not collected by the module.
# Add short local generation instructions in
{{modules/wire-compatibility-check/DEVNOTES.md}}.
The JSON schema, message ID assignment and serialization code do not change in
this ticket.
h2. When the comparison runs
The comparison is a separate job, {{check-wire}}, named {{Compare wire format
descriptions}}, in the existing {{.github/workflows/commit-check.yml}}. It runs
together with the other PR checks. It does not depend on the result of
{{check-java}}. Use the default GitHub Actions timeout; the earlier custom
30-minute limit was removed.
Use the existing {{pull_request}} trigger and repository rules for approving
external contributor runs. No separate manual action is required to run this
comparison. The job is skipped for {{push}} events because they do not provide
the PR context. The normal checks should be run early enough for the reviewer
to read the diff before final approval.
There is no target-branch name filter. PRs to {{master}} and release branches
use the same rule. This extends the original ticket description, which
mentioned only {{master}}.
h2. Which revisions are compared
Compare the target branch snapshot with the result of a test merge of the PR:
||Side||Revision||
| Base | {{github.event.pull_request.base.sha}} |
| PR merge | {{github.sha}} for the {{pull_request}} event |
The default {{actions/checkout}} behavior for a PR checks out this merge
revision. Build the two revisions in separate directories. Record both SHAs in
the Summary.
This shows what merging the PR would change relative to its target branch at
the time of the event. For example, if the target branch already contains a
wire change absent from the PR head, that change is present on both sides and
is not reported as a change made by the PR.
Do not compare the target branch directly with the PR head. Do not use
merge-base versus PR head: that was an earlier option, and the agreed choice is
now target versus PR merge. A report describes its recorded revisions; a later
target-branch update does not make that report a check of the new target tip.
h2. Generation and collection
Use Java 17 and the Maven wrapper. For each revision, build
{{wire-compatibility-check}} and its required modules:
{code:bash}
./mvnw -B -ntp clean test-compile -pl modules/wire-compatibility-check -am -T 1C
{code}
Use {{test-compile}} because the reactor needs test classes. It does not
execute the tests, so CI does not need {{-DskipTests}}. Keep {{clean}} to avoid
files left by an earlier build. Delete the old
{{modules/wire-compatibility-check/wire/}} directory before each build as well,
so the comparison cannot use JSON left in Git by an older revision.
New revisions collect JSON and write {{ids.json}} under
{{target/classes/META-INF/ignite-wire/}} in the module. {{ids.json}} maps
message IDs to message classes and must be included in the directory comparison.
The base of this PR still generates files into the old {{wire/}} directory.
Support both layouts: use the new directory when it contains {{ids.json}};
otherwise use {{wire/}}. These must be freshly generated files. The separate
shell check that failed explicitly on missing {{ids.json}} was removed by
agreement. The remaining file test selects the directory; it is not a general
validation of old generators.
Keep the existing {{MessageIdsWriter}} validation for registered messages
without descriptions. Keep {{Check all wire format descriptions are collected}}
in {{check-java}}, updated to check the new collection directory. It still
fails when a built module produces a description that was not collected. Do not
replace these checks with warnings.
h2. Results shown to the reviewer
Compare the complete directories, including added and removed files and
{{ids.json}}.
||Result||Expected behavior||
| No differences | The job succeeds. Summary says {{No wire format changes.}} |
| Differences found | The job succeeds with a warning. Summary shows the diff
preview. |
| Build, collection or comparison error | The relevant job fails. An error must
not be reported as an empty diff. |
Show at most the first 200,000 bytes of the diff in Summary. When there are
differences, upload the complete {{wire.diff}} as the {{wire-diff}} artifact
with 14-day retention. The preview must point the reviewer to the full artifact.
The presence of a diff is informational and does not fail the job. Errors from
the build or from {{diff}} itself must still fail it. Use read-only repository
permissions and do not persist checkout credentials.
h2. Local use and limits
{{modules/wire-compatibility-check/DEVNOTES.md}} must explain why the JSON
files exist, give a runnable command from the project root, state the output
directory and tell contributors not to commit the generated files. The root
{{DEVNOTES.txt}} stays unchanged.
Old release branches may lack the module, a generator or message ID generation.
Removing the target-branch filter does not add these features to old branches.
Support for each such branch needs a separate check; this ticket does not
promise full wire or ID comparison for all historical releases.
The check compares generated descriptions. It is not a semantic compatibility
analyzer and cannot guarantee detection of every change in hand-written
serialization. A PR with merge conflicts may have no {{pull_request}}
test-merge run; this ticket does not add another trigger for that case.
h2. Acceptance scenarios
* A contributor changes a message and pushes only source changes. CI generates
the JSON; no JSON update commit is required.
* A wire change produces a warning, a Summary preview with both SHAs and a full
diff artifact. A change that leaves JSON unchanged produces the no-change
message.
* A message registration change appears in {{ids.json}} in the diff.
* A PR whose branch is behind its target is compared as target versus PR merge,
so changes already in the target are not reported as PR changes.
* A PR to another target branch uses that branch's SHA. Validate this with a
branch that has the required generators.
* Both the old {{wire/}} layout and the new {{target/}} layout are generated
from scratch and can be compared during this transition.
* A build failure, an uncollected description or a technical comparison error
produces a failed check, not a successful no-change report.
* The local command and output path are documented. Generated JSON remains
outside Git.
h2. Follow-up: compatibility label through workflow_run
Label changes are deliberately outside IGNITE-29261. This PR does not change
{{check-protected-classes.yml}}, publish bot comments, add write permissions or
change the PR description.
The next task should replace the current Java-text rule for the
{{compatibility}} label with the JSON comparison result. After a successful
check of the current PR head, add the label when there is a diff and remove it
when there is no diff. A failed or cancelled check, or a result for an older PR
head, must not change the label.
The intended event chain is:
# The existing {{pull_request}} job builds, compares and publishes its result
as data in an artifact. A successful no-change result must also be available to
the label workflow; absence of a diff artifact alone is not proof of success.
# A separate {{workflow_run}} workflow with {{types: [completed]}} reads that
result, checks that it belongs to the current open PR head and updates the
label. Its code comes from the default branch. It must not check out, build or
execute PR code or artifact contents.
ASF documents the replacement of {{pull_request_target}} with this split
between a read-only build and a trusted workflow with write permissions. The
new workflow must exist on the default branch before the event chain can be
tested. The old label writer must be retired as part of that task so that two
rules do not control the same label.
Anton's earlier proposal used {{workflow_run: requested}} to set a label from
source-file changes before the build. Our later agreement uses the JSON result,
so it needs {{completed}}. Neither version requires moving the Maven comparison
itself to {{workflow_run}}.
Bot comments, automatic JSON commits, a semantic compatibility analyzer and
generator backports to old release branches are not part of this ticket. A
semantic analyzer remains a possible later step, not an agreed implementation
requirement.
h2. References
* [Broader v2 proposal on the Ignite dev
list|https://lists.apache.org/thread.html/hq3d8opxscww09pobvqfvwgc1d0v9cls].
The decisions above record the later choices and the smaller scope of this
ticket.
* [ASF GitHub Actions
policy|https://infra.apache.org/github-actions-policy.html].
* [GitHub workflow
events|https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows],
including {{pull_request}} and {{workflow_run}}.
)
> Generate and compare wire format descriptions in CI instead of tracking them
> in Git
> -----------------------------------------------------------------------------------
>
> Key: IGNITE-29261
> URL: https://issues.apache.org/jira/browse/IGNITE-29261
> Project: Ignite
> Issue Type: Improvement
> Reporter: Aleksandr Chesnokov
> Assignee: Aleksandr Chesnokov
> Priority: Major
> Fix For: 2.20
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Let's remove generated wire format JSON files from Git so contributors no
> longer need to update and commit them. As part of the normal PR checks, CI
> should generate the descriptions, including message IDs, for the target
> branch and the result of a test merge of the PR, then show their diff in
> GitHub Actions Job Summary. This should work for any target branch that has
> the required generators. Format changes should produce a warning for review,
> while build or comparison errors should fail the check. Updating the
> compatibility label through workflow_run is a separate task.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
