Skip to content

Fix bugs, replace unreliable git validation#5

Merged
petetomasik merged 19 commits intomainfrom
SUP-6490-plugin-cleanup-and-fixes
Mar 13, 2026
Merged

Fix bugs, replace unreliable git validation#5
petetomasik merged 19 commits intomainfrom
SUP-6490-plugin-cleanup-and-fixes

Conversation

@petetomasik
Copy link
Contributor

  • plugin.yml: Fixed plugin name (Commit BranchBranch Commit), description typo, removed invalid required: mandatory field, added enum/default constraints to mode
  • lib/plugin.bash: Fixed PLUGIN_PREFIX from COMMIT_BRANCH to BRANCH_COMMIT, removed 4 unused list functions
  • hooks/post-checkout: Added set -euo pipefail, quoted all variables (shell injection fix), sourced lib/plugin.bash via DIR= pattern, added BUILDKITE_COMMIT validation, mode validation, and stderr output. Replaced unreliable git name-rev with git merge-base --is-ancestor for shallow/partial clone compatibility. Handles fetch failures gracefully
    with distinct error messages
  • tests: Deleted broken tests/command.bats (wrong hook, wrong env vars, no stubs), created tests/post-checkout.bats with 10 passing tests covering both modes, early exits, missing env vars, shallow/non-shallow repos, branch mismatch, and fetch failures
  • .buildkite/pipeline.yml: Fixed linter ID from template to branch-commit, shellcheck glob from lib/** to lib/*.bash
  • README.md: Fixed invalid YAML example, documented mode as optional with strict default, added shallow clone behavior note, replaced placeholder content with actual developing/contributing sections

@petetomasik petetomasik requested a review from a team as a code owner March 12, 2026 18:46
Copy link
Contributor

@pzeballos pzeballos left a comment

Choose a reason for hiding this comment

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

LGTM

I would suggest considering splitting the logic (like, validate_env, fetch_branch, check_commit_ancestry), and the log output could be standardized with a prefix for clarity.

For example something like:

PLUGIN_LOG_PREFIX="[branch-commit-plugin]"

log_info() {
  echo "$PLUGIN_LOG_PREFIX INFO: $1" >&2
}

log_warn() {
  echo "$PLUGIN_LOG_PREFIX WARNING: $1" >&2
}

log_error() {
  echo "$PLUGIN_LOG_PREFIX ERROR: $1" >&2
}

validate_env() {
  if [[ "${BUILDKITE_SOURCE:-}" != "ui" ]]; then
    exit 0
  fi
  if [[ -z "${BUILDKITE_BRANCH:-}" ]]; then
    log_error "BUILDKITE_BRANCH environment variable is not set."
    exit 1
  fi
  if [[ -z "${BUILDKITE_COMMIT:-}" ]]; then
    log_error "BUILDKITE_COMMIT environment variable is not set."
    exit 1
  fi
}

validate_mode() {
  local mode="$1"
  if [[ "$mode" != "warn" && "$mode" != "strict" ]]; then
    log_error "Invalid mode '$mode'. Must be 'warn' or 'strict'."
    exit 1
  fi
}

...

But it's not a blocker; it's just to reduce complexity, make the script safer (by reducing repetition) and easier to extend

@petetomasik petetomasik merged commit 152593c into main Mar 13, 2026
@petetomasik petetomasik deleted the SUP-6490-plugin-cleanup-and-fixes branch March 13, 2026 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants