replace outdated scripts with scripts delegating to distribution/scripts - #3133
Conversation
|
This pull request needs "/ok-to-test" from an authorized committer. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe launchers search upward for the Membrane root, set ChangesLauncher delegation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
distribution/router/membrane.cmd (1)
9-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe root-discovery block is duplicated across all five launchers. Each launcher repeats the same upward search for
LICENSE.txtplus the runner script, the same two environment variable exports, and the same error message. A change to the discovery rule, for example adding a marker file or supporting a pre-setMEMBRANE_HOME, requires five edits. The five copies will drift.Consider extracting the discovery step into one shared script per platform, for example
distribution/scripts/find-membrane-home.cmdanddistribution/scripts/find-membrane-home.sh. A launcher cannot source a shared script before it locates the root, so keep a minimal bootstrap in each launcher and place the rest in the shared script.
distribution/router/membrane.cmd#L9-L20: replace the inline:search_upblock with a call to the shared discovery script.distribution/examples/logging/access/membrane.cmd#L9-L16: replace the inline:search_upblock with the same shared call.distribution/examples/openapi/jwt-auth/membrane.cmd#L2-L24: replace the inline:search_upblock with the same shared call.distribution/examples/logging/access/membrane.sh#L7-L15: replace the inlinewhileloop with a call to the shared Unix discovery script.distribution/examples/openapi/jwt-auth/membrane.sh#L10-L18: replace the inlinewhileloop with the same shared call.This is a maintainability improvement, not a defect. You can defer it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distribution/router/membrane.cmd` around lines 9 - 20, The launcher scripts duplicate root-discovery and environment setup logic; centralize it in shared distribution/scripts/find-membrane-home.cmd and find-membrane-home.sh scripts. In distribution/router/membrane.cmd lines 9-20, distribution/examples/logging/access/membrane.cmd lines 9-16, and distribution/examples/openapi/jwt-auth/membrane.cmd lines 2-24, retain only minimal bootstrap logic and call the shared Windows discovery script. In distribution/examples/logging/access/membrane.sh lines 7-15 and distribution/examples/openapi/jwt-auth/membrane.sh lines 10-18, replace the inline while loops with the shared Unix discovery script, preserving runner invocation and existing environment exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@distribution/examples/logging/access/membrane.cmd`:
- Line 7: Update the JAVA_OPTS assignment in membrane.cmd to quote the complete
-Dlog4j.configurationFile value, including the %SCRIPT_DIR%\log4j2_access.xml
path, so spaces in the installation directory remain intact during the later
Java invocation.
In `@distribution/examples/logging/access/membrane.sh`:
- Around line 7-15: Add tests for the root-discovery logic in
distribution/examples/logging/access/membrane.sh lines 7-15,
distribution/examples/openapi/jwt-auth/membrane.sh lines 10-18,
distribution/examples/logging/access/membrane.cmd lines 9-16,
distribution/examples/openapi/jwt-auth/membrane.cmd lines 2-24, and
distribution/router/membrane.cmd lines 9-20. Cover roots at the script directory
and several levels above, a LICENSE.txt-only directory, no root with exit status
1, and Windows install paths containing spaces; assert the discovered
MEMBRANE_HOME and exit codes, including the JAVA_OPTS behavior for the
logging/access Windows launcher.
---
Nitpick comments:
In `@distribution/router/membrane.cmd`:
- Around line 9-20: The launcher scripts duplicate root-discovery and
environment setup logic; centralize it in shared
distribution/scripts/find-membrane-home.cmd and find-membrane-home.sh scripts.
In distribution/router/membrane.cmd lines 9-20,
distribution/examples/logging/access/membrane.cmd lines 9-16, and
distribution/examples/openapi/jwt-auth/membrane.cmd lines 2-24, retain only
minimal bootstrap logic and call the shared Windows discovery script. In
distribution/examples/logging/access/membrane.sh lines 7-15 and
distribution/examples/openapi/jwt-auth/membrane.sh lines 10-18, replace the
inline while loops with the shared Unix discovery script, preserving runner
invocation and existing environment exports.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d3613be7-bd82-4017-b46b-d97b44eda4f8
📒 Files selected for processing (5)
distribution/examples/logging/access/membrane.cmddistribution/examples/logging/access/membrane.shdistribution/examples/openapi/jwt-auth/membrane.cmddistribution/examples/openapi/jwt-auth/membrane.shdistribution/router/membrane.cmd
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@distribution/examples/logging/access/membrane.sh`:
- Around line 4-5: Update the JAVA_OPTS setup in the membrane.sh example to
avoid word-splitting log4j2_access.xml paths when SCRIPT_DIR contains spaces.
Use the documented MEMBRANE_HOME-relative configuration path, or modify the
shared runner invoked by start_router.sh to preserve argument boundaries while
keeping the existing Java options behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f00ad2f5-12bd-4946-8b42-30e64010f426
📒 Files selected for processing (5)
distribution/examples/logging/access/membrane.cmddistribution/examples/logging/access/membrane.shdistribution/examples/openapi/jwt-auth/membrane.cmddistribution/examples/openapi/jwt-auth/membrane.shdistribution/router/membrane.cmd
🚧 Files skipped from review as they are similar to previous changes (3)
- distribution/examples/logging/access/membrane.cmd
- distribution/examples/openapi/jwt-auth/membrane.cmd
- distribution/router/membrane.cmd
3 participants
Summary by CodeRabbit