Skip to content

Support "dist" directory layout for szBuildVersion.json during product builds#311

Merged
barrycaceres merged 1 commit intomainfrom
caceres-version-check-2
Apr 10, 2026
Merged

Support "dist" directory layout for szBuildVersion.json during product builds#311
barrycaceres merged 1 commit intomainfrom
caceres-version-check-2

Conversation

@barrycaceres
Copy link
Copy Markdown
Contributor

  • Add fallback in findBuildVersionFile() to look for szBuildVersion.json directly under SENZING_PATH when the path ends in "dist", supporting the directory structure used during Senzing product builds
  • Retain existing lookup under {SENZING_PATH}/er/ as a fallback for completed/installed build structures
  • Use equalsIgnoreCase() for "dist" directory check to match the case-insensitive convention used elsewhere in the file
  • Add isFile() check on the "dist" path for consistency with the existing "er" path validation
  • Fix Javadoc typo "THe" to "The" in findBuildVersionFile()

…t builds

- Add fallback in findBuildVersionFile() to look for szBuildVersion.json
  directly under SENZING_PATH when the path ends in "dist", supporting
  the directory structure used during Senzing product builds
- Retain existing lookup under {SENZING_PATH}/er/ as a fallback for
  completed/installed build structures
- Use equalsIgnoreCase() for "dist" directory check to match the
  case-insensitive convention used elsewhere in the file
- Add isFile() check on the "dist" path for consistency with the
  existing "er" path validation
- Fix Javadoc typo "THe" to "The" in findBuildVersionFile()
@barrycaceres barrycaceres requested a review from a team as a code owner April 10, 2026 20:51
@barrycaceres barrycaceres self-assigned this Apr 10, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2026

Code Coverage

Overall Project 92.35% -0.13% 🟢
Files changed 64.71% 🔴

File Coverage
InstallUtilities.java 82.49% -0.96% 🔴

@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

Now I have everything needed. Here is my review.


Code Review: PR #311 — Support "dist" directory layout for szBuildVersion.json

File: src/main/java/com/senzing/sdk/core/InstallUtilities.java


Code Quality

✅ Style guide — The code follows the conventions observed in the surrounding file: consistent indentation, Javadoc format, and use of equalsIgnoreCase matching the pattern used elsewhere.

✅ No commented-out code — Inline comments are explanatory, not dead code.

✅ Meaningful variable namesversionFile, erDir are clear and consistent with the existing code.

⚠️ DRY principle — The file-existence check pattern appears twice (lines 513–515 and 525–527):

if (!versionFile.exists() || !versionFile.isFile()) {
    versionFile = null; // or return null;
}

This is minor and acceptable given the simplicity, but worth noting.

⚠️ Unnecessary comment — Line 507: // declare the version file adds no information beyond the declaration itself and is noise. Consider removing it.

⚠️ Trailing whitespace — Line 506 is a blank line with trailing spaces (visible in the diff as + ). This should be cleaned up.

✅ No defects identified — Logic flow is correct: "dist" path is tried first, with graceful fallback to the er/ path. The equalsIgnoreCase is appropriate. The surrounding try/catch for getCanonicalFile() is preserved correctly. No race conditions — this is called from a static initializer.

✅ CLAUDE.md — No environment-specific content; paths use standard conventions ($HOME, /opt/senzing) applicable to any developer.


Testing

❌ Unit tests missing — No tests were added for the new "dist" directory path. InstallUtilitiesTest.java has no coverage of findBuildVersionFile() at all, and the new branch adds two new code paths:

  1. SENZING_PATH name is "dist" and szBuildVersion.json exists → returns that file
  2. SENZING_PATH name is "dist" but file absent → falls back to er/ path

Tests should cover these cases, including the case-insensitivity of the "dist" check (e.g., "DIST", "Dist").

❌ Edge cases not covered — Specifically: "dist" directory name match with file missing (fallback path), and "dist" match with file present, are untested.


Documentation

✅ Javadoc typo fixed — "THe" → "The" (line 500).

✅ Inline comments — The two meaningful comments (// check if we might be building the Senzing product, // fall back to a completed build structure if versionFile not found) appropriately document the intent of the branching logic.

❌ CHANGELOG.md not updated — The behavior change (supporting "dist" directory layout) is user-visible in product build scenarios and should be documented in CHANGELOG.md under the current version (4.3.0).


Security

✅ No hardcoded credentials

✅ Input validationSENZING_PATH == null guard is preserved; file existence and isFile() checks are applied on both paths before use.

✅ Proper error handlingtry/catch around getCanonicalFile() is preserved; method returns null gracefully on failure.

✅ No sensitive data in logs

✅ No license files (.lic) detected


Summary

Area Status
Code style ✅ (minor: trailing whitespace + trivial comment)
No commented-out code
Meaningful names
DRY ⚠️ minor duplication
Defects
Unit tests ❌ missing for new "dist" path
Edge cases ❌ not covered
Javadoc
CHANGELOG ❌ not updated
Security

Required before merge:

  1. Add unit tests for the dist directory path (both file-found and file-missing/fallback cases).
  2. Update CHANGELOG.md.

Optional cleanup:

  • Remove the // declare the version file comment (line 507).
  • Remove trailing whitespace on the blank line at line 506.

Automated code review analyzing defects and coding standards

@barrycaceres barrycaceres enabled auto-merge (squash) April 10, 2026 20:54
@barrycaceres barrycaceres merged commit b4e177c into main Apr 10, 2026
38 checks passed
@barrycaceres barrycaceres deleted the caceres-version-check-2 branch April 10, 2026 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants