Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions bin/idstack-doctor
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,12 @@ elif [ -d "$LEGACY_DIR" ]; then
fi
if [ -f "$LEGACY_DIR/VERSION" ]; then
v=$(tr -d '[:space:]' < "$LEGACY_DIR/VERSION")
# Two arms: explicitly skip modern/future versions first, then flag the
# legacy ones. Mirrors setup. Patterns avoid literal dots and single-digit
# ranges so multi-digit components (2.0.10.0, 2.10.0.0, 20.x, 100.0.0,
# 200.0.0) work. Covered by test/test-version-classifier.sh.
case "$v" in
2.0.[1-9]*|2.[1-9]*|[3-9]*|[1-9][0-9]*) ;;
0.*|1.*|2.0.0.*|2.0.0) signature="${signature:+$signature, }VERSION=$v" ;;
esac
# Uses shared classifier to identify pre-v2.0.1.0 versions. Mirrors setup.
# Covered by test/test-version-classifier.sh.
class=$("$IDSTACK_DIR/bin/idstack-version-classifier" "$v")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Executing the helper script directly relies on the file having the executable permission bit set (chmod +x). In some environments (e.g., Windows/WSL, or when files are transferred/cloned with certain git configurations), the executable bit can be lost, leading to a "Permission denied" error.

Invoking the script with bash is more robust and avoids permission issues.

    class=$(bash "$IDSTACK_DIR/bin/idstack-version-classifier" "$v")

if [ "$class" = "legacy" ]; then
signature="${signature:+$signature, }VERSION=$v"
fi
fi
if [ -n "$signature" ]; then
echo " PROBLEM: pre-v2.0.1.0 install at $LEGACY_DIR ($signature)"
Expand Down
9 changes: 9 additions & 0 deletions bin/idstack-version-classifier
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/usr/bin/env bash
# Classifies an idstack version string as "skip" (modern), "legacy", or "unknown".
# Used by setup, bin/idstack-doctor, and test/test-version-classifier.sh.

case "${1:-}" in
2.0.[1-9]*|2.[1-9]*|[3-9]*|[1-9][0-9]*) echo "skip" ;;
0.*|1.*|2.0.0.*|2.0.0) echo "legacy" ;;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The version 2.0 is a legacy version (pre-v2.0.1.0) but it currently does not match any of the patterns, resulting in an unknown classification. This means a legacy installation with version 2.0 in its VERSION file will not be detected or cleaned up during setup.

Adding 2.0 to the legacy pattern ensures it is correctly classified.

Please also consider adding check "2.0" legacy to the test cases in test/test-version-classifier.sh to prevent regressions.

  0.*|1.*|2.0.0.*|2.0.0|2.0) echo "legacy" ;;

*) echo "unknown" ;;
esac
14 changes: 6 additions & 8 deletions setup
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,12 @@ elif [ -d "$LEGACY_SKILLS_LINK" ] && [ "$LEGACY_SKILLS_LINK" != "$IDSTACK_DIR" ]
legacy_is_old_version=0
if [ -f "$LEGACY_SKILLS_LINK/VERSION" ]; then
legacy_version=$(tr -d '[:space:]' < "$LEGACY_SKILLS_LINK/VERSION")
# Two arms: explicitly skip modern/future versions first, then flag the
# legacy ones. Patterns avoid literal dots and single-digit ranges so
# multi-digit components (2.0.10.0, 2.10.0.0, 20.x, 100.0.0, 200.0.0)
# work. Covered by test/test-version-classifier.sh.
case "$legacy_version" in
2.0.[1-9]*|2.[1-9]*|[3-9]*|[1-9][0-9]*) ;;
0.*|1.*|2.0.0.*|2.0.0) legacy_is_old_version=1 ;;
esac
# Uses shared classifier to identify pre-v2.0.1.0 versions. Covered by
# test/test-version-classifier.sh.
class=$("$IDSTACK_DIR/bin/idstack-version-classifier" "$legacy_version")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Executing the helper script directly relies on the file having the executable permission bit set. Since setup runs with set -e (line 2), any execution failure (such as "Permission denied" if the executable bit is missing) will cause the entire setup process to crash and exit immediately.

Invoking the script with bash is more robust and prevents potential installation failures.

    class=$(bash "$IDSTACK_DIR/bin/idstack-version-classifier" "$legacy_version")

if [ "$class" = "legacy" ]; then
legacy_is_old_version=1
fi
fi
if [ "$legacy_is_dispatcher" = "1" ] || [ "$legacy_is_old_version" = "1" ]; then
if [[ " $* " == *" --keep-legacy "* ]]; then
Expand Down
14 changes: 3 additions & 11 deletions test/test-version-classifier.sh
Original file line number Diff line number Diff line change
Expand Up @@ -16,26 +16,18 @@

set -e

IDSTACK_DIR="$(cd "$(dirname "$0")/.." && pwd -P)"

PASS=0
FAIL=0
TOTAL=0

# Mirror of the case statement in setup and bin/idstack-doctor. Keep these
# patterns in lockstep with both files — if you change one, change all three.
classify_version() {
case "$1" in
2.0.[1-9]*|2.[1-9]*|[3-9]*|[1-9][0-9]*) echo "skip" ;;
0.*|1.*|2.0.0.*|2.0.0) echo "legacy" ;;
*) echo "unknown" ;;
esac
}

check() {
TOTAL=$((TOTAL + 1))
local version="$1"
local expected="$2"
local got
got="$(classify_version "$version")"
got="$("$IDSTACK_DIR/bin/idstack-version-classifier" "$version")"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For consistency and robustness, invoke the helper script using bash here as well.

Suggested change
got="$("$IDSTACK_DIR/bin/idstack-version-classifier" "$version")"
got="$(bash "$IDSTACK_DIR/bin/idstack-version-classifier" "$version")"

if [ "$got" = "$expected" ]; then
PASS=$((PASS + 1))
echo " PASS: $version -> $got"
Expand Down