-
Notifications
You must be signed in to change notification settings - Fork 3
gh-15 : change shabang for compatibility and test for bash 4.3+ and bash as shell #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes update two Bash scripts to improve portability and structure. Both scripts now use a more portable shebang. The environment validation logic has been refactored into reusable functions within Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
git-artifact (1)
6-6
: Fix the typo in the error message.There's a typo: "pleaes" should be "please".
- echo "ERROR: git-artifact only runs in bash - pleaes check that the variable" + echo "ERROR: git-artifact only runs in bash - please check that the variable"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md
(2 hunks)_tests.sh
(2 hunks)git-artifact
(1 hunks)
🔇 Additional comments (9)
git-artifact (4)
1-1
: LGTM! Improved portability with environment-based shebang.The change from
#!/bin/bash
to#!/usr/bin/env bash
is a good practice for better portability across different systems where bash may be installed in different locations.
5-11
: LGTM! Good environment validation for Bash execution.The check for
BASH_VERSINFO
ensures the script is running in Bash. The implementation correctly handles the case where the variable might be unset using parameter expansion with${BASH_VERSINFO:-}
.
13-18
: LGTM! Proper Bash version compatibility check.The version check correctly ensures Bash 4.3+ compatibility by checking both major and minor version numbers. The logic
[[ "${BASH_VERSINFO[0]}" -eq 4 && "${BASH_VERSINFO[1]}" -lt 3 ]]
properly identifies Bash versions below 4.3.
26-26
: LGTM! Simplified error message.The removal of "git-subtree" reference from the error message makes it more generic and appropriate for the git-artifact context.
_tests.sh (2)
1-1
: LGTM! Consistent portability improvement.The shebang change matches the improvement made in the main
git-artifact
script, ensuring consistent portability across all scripts in the project.
11-11
: LGTM! Helpful diagnostic information.Adding bash version output is useful for debugging and ensuring test environments meet the compatibility requirements established in the main script.
README.md (3)
43-43
: LGTM! Improved contextual clarity.The updated description "The history of git-artifact workflow can basically look like this:" provides better context and clarity compared to the previous version.
63-67
: LGTM! Improved consistency in tag naming.Adding the "/bin" suffix to tags (1.1/bin, 1.2/bin, 2.0/bin) makes the example more consistent and better illustrates the recommended naming convention for git-artifact workflows.
73-75
: LGTM! Enhanced workflow example.The addition of the checkout command and new commits (2.0/src, 2.0/test) provides a more comprehensive example of how artifacts can be appended to existing tags, which aligns well with the documentation's explanation of horizontal commit history.
a761650
to
71aff64
Compare
71aff64
to
65807d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on mac by simulating various failure modes:
- using old 3.x bash by setting #!/bin/bash
- using ZSH by setting #!/bin/zsh
Both now give decent error messages.
local -n
usage #15 : change shabang for compatibility and test for bash 4.3+Summary by CodeRabbit