IGNITE-28800 Add documentation code snippets compilation check#13257
IGNITE-28800 Add documentation code snippets compilation check#13257chesnokoff wants to merge 1 commit into
Conversation
7ae1ac3 to
bb35ab3
Compare
bb35ab3 to
6dc2f2f
Compare
|
[INFO] ignite-checkstyle .................................. SUCCESS [ 0.911 s] |
anton-vinogradov
left a comment
There was a problem hiding this comment.
Comprehensive review of IGNITE-28800. Overall the change is clean and does the right thing: reparenting code-snippets to ignite-parent-internal and dropping the duplicated versions/build config is a nice simplification, and wiring a compile check into commit-check.yml will catch snippet rot going forward. A few points:
1. Stale update-versions rewrite (pom.xml, ~lines 514-522)
This PR removes the <ignite.version>…</ignite.version> property from docs/_docs/code-snippets/java/pom.xml (the version is now inherited from the parent via ${revision}). But the update-versions profile still contains an Ant replaceregexp that targets exactly that element in exactly that file:
<echo message="Update ignite.version in docs" />
<replaceregexp byline="true" encoding="UTF-8">
<regexp pattern="(<ignite\.version>).+(</ignite\.version>)" />
<substitution expression="\1${project.version}\2" />
<fileset dir="${project.basedir}/docs/">
<include name="_docs/code-snippets/java/pom.xml" />
</fileset>
</replaceregexp>After this change that block is dead — there is no longer an <ignite.version> element in the file to match, and the release version now propagates automatically through the parent's ${revision}. Please drop this now-obsolete block (keep the adjacent _config.yml rewrite, which is still needed).
2. Redundant version on ignite-kubernetes (code-snippets pom, line ~92)
ignite-kubernetes still carries <version>${ignite.version}</version>, but it is managed by ignite-bom (imported through ignite-parent-internal), so the explicit version can be dropped just like the other ignite-* dependencies in this file. For contrast, ignite-jcl (line ~46) is not in the BOM, so keeping its explicit version is correct — a one-line comment noting that would save the next reader from "fixing" it.
3. CI step cost & placement (commit-check.yml) — see inline comment.
Nit: junit-jupiter-api is pinned to 5.6.2, which is fairly old. Not blocking (it isn't managed centrally), but worth aligning with the JUnit 5 version used elsewhere in the build if there is one.
|
|
||
| - name: Documentation code snippets compilation check | ||
| run: | | ||
| ./mvnw -Pdocs -pl :code-snippets -am compile -B -V |
There was a problem hiding this comment.
Two things worth reconsidering here:
-
Redundant full recompile.
-am compilerebuilds the entire upstream dependency chain (ignite-coreplus all transitive modules) from scratch, because the profile set here (-Pdocs) differs from the previoustest-compile -Pall-java,…step, so none of the just-compiled classes are reused. That is a second full reactor build of those modules. -
Placement vs. class reuse. This step is inserted between the codestyle step and the "Run abandoned tests checks" step, and it overwrites the upstream modules'
target/classesunder a different profile than the abandoned-tests step expects (note the existing comment on that step about reusing classes / avoiding a full reactor recompile).
Suggestion: move this step to the very end of the job (after the javadoc check) so it doesn't perturb the reuse between the surrounding steps, or fold the docs profile + module into the existing test-compile reactor invocation to avoid the second recompile entirely.
|
Follow-up to my review above — ready-to-apply patch for the three points. I tried to post these as inline one-click diff --git a/.github/workflows/commit-check.yml b/.github/workflows/commit-check.yml
index 72e7f12d466..7605f36a9ac 100644
--- a/.github/workflows/commit-check.yml
+++ b/.github/workflows/commit-check.yml
@@ -99,10 +99,6 @@ jobs:
run: |
./mvnw test-compile -Pall-java,licenses,lgpl,checkstyle,examples,check-licenses -B -V -T 1C
- - name: Documentation code snippets compilation check
- run: |
- ./mvnw -Pdocs -pl :code-snippets -am compile -B -V
-
- name: Run abandoned tests checks.
# Reuse classes from the previous step; the differing profiles otherwise trigger a full reactor recompile.
run : |
@@ -112,6 +108,12 @@ jobs:
run : |
./mvnw -DskipTests install -pl modules/tools,modules/codegen -B -V && ./mvnw initialize -Pjavadoc -B -V
+ # Placed last: it recompiles the upstream modules under the `docs` profile and must not clobber the
+ # class reuse relied on by the codestyle/abandoned-tests steps above.
+ - name: Documentation code snippets compilation check
+ run: |
+ ./mvnw -Pdocs -pl :code-snippets -am compile -B -V
+
check-dotnet:
name: Сheck .NET code
runs-on: ubuntu-latest
diff --git a/docs/_docs/code-snippets/java/pom.xml b/docs/_docs/code-snippets/java/pom.xml
index 8d29c53c652..94239e27310 100644
--- a/docs/_docs/code-snippets/java/pom.xml
+++ b/docs/_docs/code-snippets/java/pom.xml
@@ -42,6 +42,7 @@
</dependency>
<dependency>
<groupId>${project.groupId}</groupId>
+ <!-- ignite-jcl is not part of ignite-bom, so the version must be specified explicitly. -->
<artifactId>ignite-jcl</artifactId>
<version>${ignite.version}</version>
</dependency>
@@ -89,7 +90,6 @@
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>ignite-kubernetes</artifactId>
- <version>${ignite.version}</version>
</dependency>
diff --git a/pom.xml b/pom.xml
index da005ab74d9..ea2d5a66e04 100644
--- a/pom.xml
+++ b/pom.xml
@@ -513,14 +513,6 @@
<target>
<echo message="Update ignite.version in docs" />
- <replaceregexp byline="true" encoding="UTF-8">
- <regexp pattern="(<ignite\.version>).+(</ignite\.version>)" />
- <substitution expression="\1${project.version}\2" />
- <fileset dir="${project.basedir}/docs/">
- <include name="_docs/code-snippets/java/pom.xml" />
- </fileset>
- </replaceregexp>
-
<replaceregexp byline="true" encoding="UTF-8">
<regexp pattern="(version: ).+" />
<substitution expression="\1${project.version}" />Summary:
|
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat TC.Bot - Instance 1 or TC.Bot - Instance 2)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.