Skip to content

Conversation

@lavanyavijayk
Copy link

@lavanyavijayk lavanyavijayk commented Jan 14, 2025

  • Improved version management by introducing Maven's property
  • Replaced hardcoded version strings with dynamic retrieval
  • Added version.properties to support runtime version access
  • Ensured updated version in scripts and classes

Purpose of the PR

This PR centralizes version management across the project by leveraging Maven's ${revision} property and a filtered version.properties file for dynamic runtime access to the version. It removes the need for hardcoded version strings and ensures consistent versioning across all modules.

Main Changes

  • Replaced hardcoded version strings in classes like CommonVersion, RpcVersion, CoreVersion, and API.
  • Added a version.properties file with resource filtering for dynamic version retrieval.
  • Updated startup scripts (start-pd.sh, start-store.sh) to reflect the new version.
  • Ensured compliance with Apache license requirements for new files.
  • Refactored the version handling logic to avoid redundant version updates in code.

Need More Clarity on

file path - /hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/version/ApiVersion.java had a change mentioned in the ticket but not sure which string needs to be replaced with the revision value. need confirmation to go ahead and make the change.

##Working On
Attempted to update CoreVersion.DEFAULT_VERSION(file path - /hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/version/CoreVersion.java) to dynamically use the revision value, but encountered an error during compilation, so the change is not yet applied.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • [x ] Need tests and can be verified as follows:
    • xxx

Does this PR potentially affect the following parts?

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

- Improved version management by introducing Maven's  property
- Replaced hardcoded version strings with dynamic retrieval
- Added version.properties to support runtime version access
- Ensured updated version in scripts and classes
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jan 14, 2025
@imbajin imbajin changed the title Centralize version management. refactor: centralize version management in all Jan 14, 2025
@imbajin imbajin changed the title refactor: centralize version management in all refactor: centralize version management in all modules Jan 14, 2025
@VGalaxies
Copy link
Contributor

@lavanyavijayk There is an issue with the version number here for compilation error, it should be updated to ${hugegraph-commons.version}.

@lavanyavijayk
Copy link
Author

lavanyavijayk commented Jan 18, 2025

@lavanyavijayk There is an issue with the version number here for compilation error, it should be updated to ${hugegraph-commons.version}.
@VGalaxies Thank you for pointing that out. I have made the changes.

I also want to get more details for the following.

The ticket mentions a change in /hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/version/ApiVersion.java, but it is unclear which string should be replaced with the revision value. Could you confirm this before proceeding?
I attempted to dynamically assign the revision value to CoreVersion.DEFAULT_VERSION (/hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/version/CoreVersion.java), but it caused a compilation error. The issue arises because CoreVersion.DEFAULT_VERSION is also used as an annotation value in /hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/server/ApplicationConfig.java, which requires a constant expression. Should I defer this change for now?

@codecov
Copy link

codecov bot commented Jan 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.94%. Comparing base (a4cb44e) to head (f487fbd).

❗ There is a different number of reports uploaded between BASE (a4cb44e) and HEAD (f487fbd). Click for more details.

HEAD has 6 uploads less than BASE
Flag BASE (a4cb44e) HEAD (f487fbd)
7 1
Additional details and impacted files
@@              Coverage Diff              @@
##             master    #2722       +/-   ##
=============================================
- Coverage     46.98%   34.94%   -12.04%     
+ Complexity      821      382      -439     
=============================================
  Files           745      745               
  Lines         60060    60060               
  Branches       7669     7669               
=============================================
- Hits          28218    20989     -7229     
- Misses        29018    36822     +7804     
+ Partials       2824     2249      -575     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link

Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label

@imbajin imbajin requested a review from Copilot March 30, 2025 10:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR centralizes version management by replacing hardcoded version strings with dynamic retrieval from Maven's revision property via a filtered version.properties file.

  • Replaces hardcoded version strings in API.java, RpcVersion.java, and CommonVersion.java with a call to VersionUtil.getVersionNumber().
  • Introduces getVersionNumber() in VersionUtil.java to load the version dynamically from version.properties.
  • Updates startup scripts (not shown here) and ensures compliance with Apache license requirements.

Reviewed Changes

Copilot reviewed 4 out of 9 changed files in this pull request and generated no comments.

File Description
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/API.java Replaces hardcoded version with dynamic retrieval via VersionUtil.
hugegraph-commons/hugegraph-rpc/src/main/java/org/apache/hugegraph/version/RpcVersion.java Updates version initialization to use dynamic version from VersionUtil.
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/version/CommonVersion.java Updates version initialization to use dynamic version from VersionUtil.
hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/util/VersionUtil.java Adds a method to load the version from a properties file.
Files not reviewed (5)
  • hugegraph-commons/hugegraph-common/pom.xml: Language not supported
  • hugegraph-commons/hugegraph-common/src/main/resources/version.properties: Language not supported
  • hugegraph-pd/hg-pd-service/pom.xml: Language not supported
  • hugegraph-server/hugegraph-dist/src/assembly/travis/start-pd.sh: Language not supported
  • hugegraph-server/hugegraph-dist/src/assembly/travis/start-store.sh: Language not supported
Comments suppressed due to low confidence (1)

hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/util/VersionUtil.java:242

  • Consider adding a null check for the InputStream before calling PROPS.load(is) to prevent a potential NullPointerException in case version.properties is not found.
try (InputStream is = VersionUtil.class.getResourceAsStream("/version.properties")) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inactive size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants