feat: 支持模块启动时通过传递env参数动态指定模块的启动类 (#999) #1029
Conversation
|
Hi @liufeng-xiwo, welcome to SOFAStack community, Please sign Contributor License Agreement! After you signed CLA, we will automatically sync the status of this pull request in 3 minutes. |
sofastack-cla
bot
added
bug
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@liufeng-xiwo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java (1)
206-206: LGTM! Consider adding a Javadoc comment for better maintainability.The new constant
BIZ_MAIN_CLASSis well-defined and follows the established naming conventions. Its purpose aligns with the PR objective of supporting dynamic module startup class specification.Consider adding a Javadoc comment to document the purpose and usage of this constant, similar to other sections in the file. Example:
+ /** + * Environment property to dynamically specify the module's startup class + */ public final static String BIZ_MAIN_CLASS = "sofa.ark.biz.main.class";sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (1)
326-332: Add validation and documentation for the main class override feature.While the implementation allows for dynamic specification of the main class, consider these improvements:
- Validate that
mainClassFromEnvcontains a valid class name- Add Javadoc to document this feature in the
startanddoStartmethods// 支持运行时根据环境变量动态设置模块的启动类 String mainClassFromEnv = envs.get(Constants.BIZ_MAIN_CLASS); -if (mainClassFromEnv != null){ +if (!StringUtils.isEmpty(mainClassFromEnv)) { + // Validate class name format + AssertUtils.isFalse(StringUtils.isEmpty(mainClassFromEnv), "Main Class from environment must not be empty!"); mainClass = mainClassFromEnv; ArkLoggerFactory.getDefaultLogger().info("Ark biz {} will start with main class {} from envs", getIdentity(), mainClassFromEnv); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java(1 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java(1 hunks)
🔇 Additional comments (1)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (1)
333-333: LGTM: Appropriate error handling for missing main class.
The error handling is well implemented with a descriptive error message that includes the biz name for better debugging context.
|
|
||
| // 支持运行时根据环境变量动态设置模块的启动类 | ||
| String mainClassFromEnv = envs.get(Constants.BIZ_MAIN_CLASS); | ||
| if (mainClassFromEnv != null){ | ||
| mainClass = mainClassFromEnv; | ||
| ArkLoggerFactory.getDefaultLogger().info("Ark biz {} will start with main class {} from envs", getIdentity(), mainClassFromEnv); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Test coverage needed for environment-based main class configuration
The verification reveals:
- The
BIZ_MAIN_CLASSconstant is properly defined inConstants.java - The only usage of this constant is in the new implementation
- No test coverage found for this new environment-based main class configuration feature
Action items:
- Add test cases to verify the environment-based main class configuration
- Include both positive (when env var is set) and negative (when env var is not set) test scenarios
- Test the logging behavior when environment variable is used
🔗 Analysis chain
Verify constant usage and test coverage.
Let's verify the implementation of this feature:
- Check if the
BIZ_MAIN_CLASSconstant is properly defined - Look for test coverage of this new functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for constant definition
echo "Checking for BIZ_MAIN_CLASS constant definition..."
rg -p "BIZ_MAIN_CLASS.*=.*\"sofa.ark.biz.main.class\""
# Check for test coverage
echo "Checking for test coverage..."
fd -e java -e groovy -e scala test | rg -l "BIZ_MAIN_CLASS|mainClassFromEnv"
Length of output: 516
Script:
#!/bin/bash
# Let's check for test files that might cover BizModel or main class configuration
echo "Checking for BizModel test coverage..."
rg -p "class.*Test.*BizModel" -g "*.java"
# Check for any test files that might test environment variable configuration
echo "Checking for environment variable test coverage..."
rg -p "test.*[Ee]nv.*main.*class" -g "*.java"
# Check for actual usage of this constant in the codebase
echo "Checking for usage of BIZ_MAIN_CLASS constant..."
rg -p "Constants\.BIZ_MAIN_CLASS" -A 2 -g "*.java"
Length of output: 712
liufeng-xiwo
force-pushed
the
issue999
branch
2 times, most recently
from
33f3319
to
dc93ed7
Compare
liufeng-xiwo
force-pushed
the
issue999
branch
from
dc93ed7
to
e1c3dd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (3)
318-318: Convert comment to English for consistencyThe comment should be in English to maintain consistency with the codebase.
- // 传 env 参数，使用动态的模块启动类 + // Pass env parameter to use dynamic module startup class
315-330: Enhance test coverage for dynamic main class featureWhile the happy path is tested, consider adding test cases for:
- Invalid main class name
- Empty main class name
- Non-existent main class
This would ensure the feature handles error cases gracefully.
Example addition:
@Test public void testInstallOperationWithInvalidDynamicMainClass() throws Throwable { BizOperation bizOperation = new BizOperation(); bizOperation.setOperationType(INSTALL); bizOperation.getParameters().put(CONFIG_BIZ_URL, bizUrl5.toString()); bizOperation.setBizName("biz-demo"); bizOperation.setBizVersion("5.0.0"); // Test with non-existent class Map<String, String> envs = Collections.singletonMap(Constants.BIZ_MAIN_CLASS, "org.example.NonExistentClass"); ClientResponse response = installOperation(bizOperation, new String[] {}, envs); // Add appropriate assertions based on expected behavior }
327-329: Make assertions more descriptiveThe assertions could be more descriptive to better document the test's intent and make failures more informative.
- ClientResponse response2 = installOperation(bizOperation, new String[] {}, envs); - assertEquals(SUCCESS, response2.getCode()); - assertEquals("org.example.Main2", (new ArrayList<>(response2.getBizInfos())).get(0).getMainClass()); + ClientResponse response = installOperation(bizOperation, new String[] {}, envs); + assertEquals("Installation should succeed", SUCCESS, response.getCode()); + BizInfo bizInfo = (new ArrayList<>(response.getBizInfos())).get(0); + assertEquals("Main class should be set to the specified class", + "org.example.Main2", + bizInfo.getMainClass());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
sofa-ark-parent/core-impl/container/src/test/resources/sample-ark-5.0.0-ark-biz.jaris excluded by!**/*.jar
📒 Files selected for processing (3)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java(1 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java(5 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java
liufeng-xiwo
force-pushed
the
issue999
branch
from
eaed6ae
to
6833d9c
Compare
liufeng-xiwo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (1)
322-349: Good test implementation, but some improvements suggested.The test comprehensively validates the dynamic main class functionality. However, consider these improvements:
- Replace Chinese comments with English for international collaboration
- Extract magic strings as constants
- Add test cases for error scenarios
Consider applying these improvements:
- // 传 env 参数，使用动态的模块启动类 org.example.Main2 + // Test dynamic module startup class using env parameter + private static final String DYNAMIC_MAIN_CLASS = "org.example.Main2"; + private static final String ORIGINAL_MAIN_CLASS = "org.example.Main1"; BizOperation bizOperation = new BizOperation(); bizOperation.setOperationType(INSTALL); bizOperation.getParameters().put(CONFIG_BIZ_URL, bizUrl5.toString()); bizOperation.setBizName("biz-demo"); bizOperation.setBizVersion("5.0.0"); - Map<String, String> envs = Collections.singletonMap(Constants.BIZ_MAIN_CLASS, "org.example.Main2"); + Map<String, String> envs = Collections.singletonMap(Constants.BIZ_MAIN_CLASS, DYNAMIC_MAIN_CLASS); ClientResponse response2 = installOperation(bizOperation, new String[] {}, envs); assertEquals(SUCCESS, response2.getCode()); - assertEquals("org.example.Main2", (new ArrayList<>(response2.getBizInfos())).get(0).getMainClass()); + assertEquals(DYNAMIC_MAIN_CLASS, (new ArrayList<>(response2.getBizInfos())).get(0).getMainClass()); - // 实际模块jar包里配置启动类是 org.example.Main1 + // Verify original main class in module JAR manifestConsider adding these test cases:
- Test with invalid main class
- Test with empty/null main class
- Test with non-existent main class
Would you like me to help implement these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
sofa-ark-parent/core-impl/container/src/test/resources/sample-ark-5.0.0-ark-biz.jaris excluded by!**/*.jar
📒 Files selected for processing (3)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java(1 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java(5 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java
🔇 Additional comments (2)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (2)
106-107: LGTM! Field declaration follows existing patterns.
The new field bizUrl5 is well-documented and maintains consistency with the existing field declarations.
120-121: LGTM! Field initialization is consistent.
The initialization of bizUrl5 follows the established pattern and maintains consistency with other URL initializations.
liufeng-xiwo
force-pushed
the
issue999
branch
from
6833d9c
to
50b6b6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (2)
322-349: LGTM! Consider adding more test cases for comprehensive coverage.The test effectively validates the dynamic main class specification feature. Consider adding these test cases for more thorough coverage:
- Negative test case when the specified main class doesn't exist
- Test behavior when environment parameter is not set
- Test with invalid main class values
341-347: Ensure proper resource cleanup.The test creates temporary files and opens JarFile resources. While the InputStream is properly closed using try-with-resources, the JarFile and JarFileArchive resources should also be closed.
Apply this diff to ensure proper resource cleanup:
- JarFile bizFile = new JarFile(file); - JarFileArchive jarFileArchive = new JarFileArchive(bizFile); - BizArchive bizArchive = new JarBizArchive(jarFileArchive); - assertEquals("org.example.Main1", bizArchive.getManifest().getMainAttributes().getValue(Constants.MAIN_CLASS_ATTRIBUTE)); - assertEquals("org.example.Main1", bizArchive.getManifest().getMainAttributes().getValue(Constants.START_CLASS_ATTRIBUTE)); + try (JarFile bizFile = new JarFile(file)) { + JarFileArchive jarFileArchive = new JarFileArchive(bizFile); + BizArchive bizArchive = new JarBizArchive(jarFileArchive); + assertEquals("org.example.Main1", bizArchive.getManifest().getMainAttributes().getValue(Constants.MAIN_CLASS_ATTRIBUTE)); + assertEquals("org.example.Main1", bizArchive.getManifest().getMainAttributes().getValue(Constants.START_CLASS_ATTRIBUTE)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
sofa-ark-parent/core-impl/container/src/test/resources/sample-ark-5.0.0-ark-biz.jaris excluded by!**/*.jar
📒 Files selected for processing (3)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java(1 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java(5 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java
🔇 Additional comments (1)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (1)
106-107: LGTM! Variable declaration and initialization are consistent.
The new URL variable bizUrl5 follows the established pattern and is properly initialized in the before() method.
Also applies to: 120-121
模块启动时，可以通过 envs["sofa.ark.biz.main.class"] 参数传递指定的类作为模块的启动类
Summary by CodeRabbit
New Features
BIZ_MAIN_CLASSfor improved configuration options related to business logic.Bug Fixes