[Feature] pre-push hook을 통한 변경 모듈 대상 lint 및 정적분석 실행#1330
[Feature] pre-push hook을 통한 변경 모듈 대상 lint 및 정적분석 실행#1330
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough새로운 Bash(pre-push) 훅 Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Git as Git
participant Hook as Pre-push Hook
participant Repo as Repository\\n(settings.gradle)
participant Gradle as Gradle / Lint Tools
Dev->>Git: git push
Git->>Hook: trigger pre-push
Hook->>Git: determine merge-base (origin/develop -> develop)
Hook->>Git: collect changed files since merge-base
Hook->>Repo: parse settings.gradle -> module list
alt merge-base 불가 OR 루트 Gradle/빌드 파일 변경
Hook->>Gradle: run spotlessCheck, ktlintCheck, detekt (full)
else 변경된 Kotlin 관련 모듈 존재
Hook->>Hook: map changed files -> affected modules
Hook->>Gradle: run root spotlessCheck
Hook->>Gradle: run :module:ktlintCheck & :module:detekt for each module (--continue)
else 관련 변경 없음
Hook->>Dev: exit early (no tasks)
end
Gradle->>Dev: report results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (1)
.githooks/pre-push (1)
1-1: Bash 4+ 버전 호환성 고려 필요
declare -A(연관 배열)는 Bash 4.0 이상에서만 지원됩니다. macOS의 기본 Bash는 3.x 버전이므로, Homebrew 등으로 설치한 최신 Bash를 사용하지 않는 개발자에게 오류가 발생할 수 있습니다.다음 옵션 중 하나를 고려해 보세요:
- 스크립트 시작 부분에 버전 체크 추가
- 또는 README/설정 가이드에 Bash 4+ 요구사항 명시
♻️ 버전 체크 추가 예시
#!/bin/bash # pre-push hook: run ktlintCheck, spotlessCheck, detekt for changed modules only +if ((BASH_VERSINFO[0] < 4)); then + echo "[pre-push] Error: Bash 4.0+ required for associative arrays. Current: $BASH_VERSION" + echo "[pre-push] On macOS, install via: brew install bash" + exit 1 +fi + set -e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.githooks/pre-push at line 1, The pre-push hook uses Bash features (e.g., declare -A associative arrays) that require Bash 4+, so add a runtime version check right after the existing shebang (#!/bin/bash) to detect bash major version <4 and print a clear, actionable error recommending installing/updating Bash (or exit non‑zero), or alternatively document the Bash 4+ requirement in the project README; specifically, check BASH_VERSINFO[0] or parse "$(bash --version)" and fail early before any use of declare -A or other Bash 4+ features.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.githooks/pre-push:
- Line 34: The line containing dir="${dir#/}" has an extraneous trailing
character "n" causing a "n: command not found" error; remove the stray "n" (and
any extra whitespace after dir="${dir#/}") so the line is just dir="${dir#/}",
ensuring the variable assignment in the .githooks/pre-push hook is clean and no
unexpected token remains.
---
Nitpick comments:
In @.githooks/pre-push:
- Line 1: The pre-push hook uses Bash features (e.g., declare -A associative
arrays) that require Bash 4+, so add a runtime version check right after the
existing shebang (#!/bin/bash) to detect bash major version <4 and print a
clear, actionable error recommending installing/updating Bash (or exit
non‑zero), or alternatively document the Bash 4+ requirement in the project
README; specifically, check BASH_VERSINFO[0] or parse "$(bash --version)" and
fail early before any use of declare -A or other Bash 4+ features.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d3d12fd-7c0c-429f-b1c8-76ce9767591a
📒 Files selected for processing (1)
.githooks/pre-push
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ Actions performedReview triggered.
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| MODULE_PATHS["$token"]="$dir" | ||
| fi | ||
| done | ||
| done < <(grep "^include" settings.gradle) |
There was a problem hiding this comment.
🟠 [Major] settings.gradle 파싱 - 모듈 인식 범위 제한
현재 파싱 로직은 Groovy 스타일의 include ':module' 형식만 처리합니다. 이 프로젝트의 경우 settings.gradle(Groovy)을 사용하므로 현재는 동작하지만, 다음 두 가지 취약점이 있습니다.
1. 멀티라인 include 미처리:
// 이런 형식은 파싱 불가
include ':feature:timetable',
':feature:bus',
':feature:dining'2. for token in $line - 따옴표 없는 확장 (glob expansion 위험):
# $line에 glob 문자(*, ?, [)가 포함되면 의도치 않은 파일명 확장 발생 가능
# 개선안: IFS를 활용한 명시적 토큰 파싱
while IFS= read -r line; do
# include ':a', ':b' 형식을 처리
if [[ "$line" =~ ^include ]]; then
while read -r token; do
token="${token//[\'\"()]/}"
token="${token//,/}"
token="${token// /}"
if [[ "$token" == :* ]]; then
dir="${token//:/\/}"
dir="${dir#/}"
MODULE_PATHS["$token"]="$dir"
fi
done <<< "${line#include}"
fi
done < settings.gradle
|
|
||
| while IFS= read -r file; do | ||
| case "$file" in | ||
| build-logic/*|build.gradle.kts|gradle.properties|settings.gradle|gradle/libs.versions.toml) |
There was a problem hiding this comment.
🟠 [Major] 루트 build.gradle 변경 시 전체 검사 미실행
현재 프로젝트는 루트 빌드 파일로 build.gradle(Groovy 스타일)을 사용하고 있습니다. 하지만 case 패턴에는 build.gradle.kts만 포함되어 있어, 루트 build.gradle 변경 시 전체 검사가 트리거되지 않습니다.
또한 .github/workflows/*.yml 같은 CI 설정 변경도 전체 검사 대상에 포함할지 고려해보세요.
| build-logic/*|build.gradle.kts|gradle.properties|settings.gradle|gradle/libs.versions.toml) | |
| build-logic/*|build.gradle|build.gradle.kts|gradle.properties|settings.gradle|settings.gradle.kts|gradle/libs.versions.toml) |
| exit 0 | ||
| fi | ||
|
|
||
| CHANGED_FILES=$(git diff --name-only "$MERGE_BASE" HEAD) |
There was a problem hiding this comment.
🟡 [Minor] git diff 범위 - pre-push hook의 표준 입력 미활용
현재 git diff --name-only "$MERGE_BASE" HEAD는 현재 커밋까지만 비교합니다. 하지만 git push는 여러 커밋을 한 번에 push할 수 있으며, pre-push hook은 stdin으로 push 대상 ref 정보를 받습니다.
예를 들어 로컬에서 stash 또는 uncommitted changes가 있다면 해당 변경사항은 검사되지 않습니다.
Git pre-push hook의 표준적인 패턴:
# stdin에서 push 대상 정보 읽기
while read local_ref local_sha remote_ref remote_sha; do
if [ "$local_sha" = "0000000000000000000000000000000000000000" ]; then
continue # 브랜치 삭제는 스킵
fi
REMOTE_SHA="$remote_sha"
done
# 이미 remote에 있는 커밋은 제외
if [ "$REMOTE_SHA" != "0000000000000000000000000000000000000000" ]; then
CHANGED_FILES=$(git diff --name-only "$REMOTE_SHA" HEAD)
else
CHANGED_FILES=$(git diff --name-only "$MERGE_BASE" HEAD)
fi현재 접근 방식도 실용적으로 동작하지만, 정확성을 위해 stdin 활용을 고려해보세요.
| echo "[pre-push] Affected modules: ${!MODULES[*]}" | ||
|
|
||
| # spotlessCheck is root-level only (target "**/*.kt" across all modules) | ||
| TASKS=("spotlessCheck") |
There was a problem hiding this comment.
🟡 [Minor] Kotlin 파일 변경 여부 필터링 미구현
PR 설명에 "Kotlin 관련 변경이 없으면 조기 종료"라고 기술되어 있지만, 실제 구현에는 이 기능이 없습니다.
현재는 모듈 내 어떤 파일이든 변경되면 (XML 레이아웃, 리소스, 이미지 등) ktlintCheck와 detekt가 실행됩니다. Kotlin 파일 변경이 없는 경우 불필요한 Gradle 실행 오버헤드가 발생합니다.
개선 방안:
# Kotlin 파일 변경 여부 확인
KOTLIN_CHANGED=$(echo "$CHANGED_FILES" | grep -E '\.kt$' || true)
if [ -z "$KOTLIN_CHANGED" ]; then
echo "[pre-push] No Kotlin files changed. Skipping ktlint/detekt checks."
# spotlessCheck는 다른 파일도 커버하므로 별도 처리 필요
exit 0
fi단, spotlessCheck는 Kotlin 외 파일도 포함하므로 로직을 분리하여 처리하면 더욱 효율적입니다.
| #!/usr/bin/env bash | ||
| # pre-push hook: run ktlintCheck, spotlessCheck, detekt for changed modules only | ||
|
|
||
| set -e |
There was a problem hiding this comment.
🟡 [Minor] set -e + 파이프라인 조합 주의
set -e는 명령 실패 시 즉시 종료하는 좋은 관행입니다. 다만 파이프라인과 함께 사용할 때는 set -o pipefail도 함께 사용하는 것이 권장됩니다.
현재 echo "$CHANGED_FILES" | sed 's/^/ /' (line 30)처럼 파이프라인을 사용하는 경우, pipefail 없이는 sed 이전 명령의 실패를 감지하지 못합니다.
| set -e | |
| set -eo pipefail |
| done | ||
|
|
||
| echo "[pre-push] Running: ./gradlew ${TASKS[*]}" | ||
| ./gradlew --continue "${TASKS[@]}" |
There was a problem hiding this comment.
🔵 [Info] --continue 플래그 - 긴급 push 우회 메커니즘 부재
--continue 플래그로 모든 태스크를 실행하는 것은 전체 오류를 확인하는 데 유용합니다. 그러나 현재 hook을 우회하는 방법이 없어 긴급 상황에서 개발자가 곤란할 수 있습니다.
권장 개선사항 - 스킵 메커니즘 추가:
# 스크립트 상단에 추가
if [ "${SKIP_HOOKS:-}" = "1" ] || [ "${SKIP_PRE_PUSH:-}" = "1" ]; then
echo "[pre-push] Hook skipped via SKIP_HOOKS env variable."
exit 0
fi사용법:
SKIP_HOOKS=1 git push
# 또는 표준 git 방식 (단, hooks 전체 비활성화)
git push --no-verify--no-verify는 이미 git 자체에서 지원하므로, 최소한 README에 이 방법을 문서화하는 것이 좋습니다.
| exit 1 | ||
| fi | ||
|
|
||
| MAIN_BRANCH="origin/develop" |
There was a problem hiding this comment.
🔵 [Info] MAIN_BRANCH 하드코딩 - sprint 브랜치 대응 불가
현재 origin/develop으로 하드코딩되어 있어 sprint 브랜치로 push하는 경우 merge-base가 정확히 계산되지 않을 수 있습니다.
PR 설명에 따르면 develop과 sprint 브랜치를 모두 대상으로 하므로, 현재 브랜치의 upstream tracking branch를 동적으로 감지하는 방식을 고려해볼 수 있습니다:
# 동적으로 upstream 브랜치 감지
UPSTREAM=$(git rev-parse --abbrev-ref --symbolic-full-name @{u} 2>/dev/null || echo "origin/develop")
MERGE_BASE=$(git merge-base HEAD "$UPSTREAM" 2>/dev/null || \
git merge-base HEAD origin/develop 2>/dev/null || \
git merge-base HEAD develop 2>/dev/null || true)현재 코드도 fallback이 있어 큰 문제는 없지만, 의도를 명확히 표현하는 데 도움이 됩니다.
| if [[ "$token" == :* ]]; then | ||
| dir="${token//://}" |
There was a problem hiding this comment.
🔵 [Info] 잘 작성된 모듈 경로 변환 로직
:core:notification → core/notification 변환 로직이 깔끔합니다.
${token//://}: 모든:→/치환 (:core:notification→/core/notification)${dir#/}: 선행/제거 →core/notification
이후 "$file" == "$dir/"* 패턴 매칭과 조합하여 중첩 모듈의 longest-match를 올바르게 처리하려는 설계 의도는 좋습니다. 다만 라인 62-64의 버그로 인해 현재는 동작하지 않습니다. (Critical 코멘트 참조)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.githooks/pre-push:
- Around line 62-64: The conditional uses an incorrect backtick-wrapped
expression `${\`#dir\`}` which attempts command substitution instead of bash
parameter expansion; change references to use the correct length expansion
`${`#dir`}` wherever it's used (e.g., in the if condition comparing to
`matched_len` and any assignment to `matched_len`), ensuring variables `dir`,
`matched_len`, and `matched_module` remain unchanged and the comparison uses
integer contexts.
🪄 Autofix (Beta)
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
Run ID: 066a14a1-ffa7-4ed7-afa1-fecd95e079a4
📒 Files selected for processing (1)
.githooks/pre-push
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.githooks/pre-push:
- Around line 86-91: The pre-push hook currently unconditionally appends
"${module}:detekt" for every module (in the TASKS array loop over MODULES),
which fails for the domain module because it doesn't have a detekt task; either
change the hook to conditionally add detekt only if the task exists (e.g., check
Gradle for ":${module}:tasks" or probe for ":${module}:detekt" before
TASKS+=("${module}:detekt")) or apply the detekt plugin to the domain module so
":domain:detekt" exists; update the loop that builds TASKS to use this
conditional check or add the detekt plugin to the domain module configuration
accordingly.
🪄 Autofix (Beta)
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
Run ID: e070e041-bb72-4ad2-9272-948b189af14c
📒 Files selected for processing (1)
.githooks/pre-push
| # spotlessCheck is root-level only (target "**/*.kt" across all modules) | ||
| TASKS=("spotlessCheck") | ||
| for module in "${!MODULES[@]}"; do | ||
| TASKS+=("${module}:ktlintCheck") | ||
| TASKS+=("${module}:detekt") | ||
| done |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if domain module has detekt task
echo "=== Checking domain module's build.gradle.kts ==="
cat domain/build.gradle.kts 2>/dev/null || cat domain/build.gradle 2>/dev/null
echo ""
echo "=== Checking JavaLibraryConventionPlugin for detekt plugin ==="
fd -t f "JavaLibraryConventionPlugin.kt" build-logic --exec cat {}
echo ""
echo "=== Checking all convention plugins for detekt usage ==="
rg -n "detekt" build-logic/convention/src/main/java/Repository: BCSDLab/KOIN_ANDROID
Length of output: 2655
domain 모듈에 detekt 태스크가 없습니다.
domain 모듈은 JavaLibraryConventionPlugin을 사용하며, 이 플러그인은 detekt를 적용하지 않습니다. domain/ 파일이 변경되면 :domain:detekt 태스크 실행 시 오류가 발생합니다.
조건부 실행으로 도메인 모듈의 detekt 태스크를 건너뛰거나, 도메인 모듈에 detekt 플러그인을 적용하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.githooks/pre-push around lines 86 - 91, The pre-push hook currently
unconditionally appends "${module}:detekt" for every module (in the TASKS array
loop over MODULES), which fails for the domain module because it doesn't have a
detekt task; either change the hook to conditionally add detekt only if the task
exists (e.g., check Gradle for ":${module}:tasks" or probe for
":${module}:detekt" before TASKS+=("${module}:detekt")) or apply the detekt
plugin to the domain module so ":domain:detekt" exists; update the loop that
builds TASKS to use this conditional check or add the detekt plugin to the
domain module configuration accordingly.
2 participants
PR 개요
이슈 번호: #1320
PR 체크리스트
작업사항
작업사항의 상세한 설명
push 전에 변경된 모듈만 감지하여 ktlintCheck, spotlessCheck, detekt를 실행하는 pre-push Git hook을 추가했습니다. spotlessCheck는 root 레벨에서 전체 파일을 대상으로, ktlintCheck와 detekt는 변경된 모듈에 대해서만 실행됩니다.
모듈 목록을 settings.gradle에서 동적으로 파싱하도록 설계하여, 새 모듈이 추가되어도 hook 파일을 별도로 수정할 필요가 없습니다.
추가내용
Summary by CodeRabbit