Skip to content

fix: PR 코드 리뷰 개선 사항 반영 (issues #27, #30, #37)#43

Merged
ppzxc merged 3 commits into
mainfrom
issue/fix-27-30-37
Mar 24, 2026
Merged

fix: PR 코드 리뷰 개선 사항 반영 (issues #27, #30, #37)#43
ppzxc merged 3 commits into
mainfrom
issue/fix-27-30-37

Conversation

@ppzxc

@ppzxc ppzxc commented Mar 24, 2026

Copy link
Copy Markdown
Owner

Summary

Motivation

PR #26, #29, #36 코드 리뷰에서 발견된 3가지 개선 사항을 반영한다.

Changes

  • internal/adapter/input/http/middleware.go: ok/not-ok 패턴으로 type assertion 변경, 명시적 패닉 메시지 추가
  • internal/adapter/output/webhook/sender.go: &http.Transport{}http.DefaultTransport.(*http.Transport).Clone()
  • cmd/server/main.go: buildRelayWorkerConfig 반환 타입을 (RelayWorkerConfig, error)로 변경, 호출부에서 에러 처리

Tidy First

Test plan

  • 빌드 확인 (go build ./cmd/server/)
  • 전체 테스트 통과 (go test -race ./... -timeout 60s)
  • 정적 분석 통과 (go vet ./...)
  • TestInputTypeFromContext_PanicsWithMessage — 명시적 패닉 메시지 검증
  • TestBuildRelayWorkerConfig_InvalidRetryDelay — 잘못된 duration fail-fast 검증
  • TestBuildRelayWorkerConfig_InvalidPollBackoff — 잘못된 duration fail-fast 검증

Closes #27
Closes #30
Closes #37

@ppzxc ppzxc left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

코드 리뷰 결과: APPROVE (자기 PR 승인 불가 → 코멘트로 대체)

전반적으로 이슈 의도에 충실하고 Tidy First 원칙(structural → behavioral 분리)이 잘 적용되었습니다.

#30 — sender.go
DefaultTransport.(*http.Transport).Clone() 패턴은 올바른 관용적 표현입니다. DefaultTransport가 외부에서 교체된 환경에서는 type assertion 패닉이 가능하나, 프로덕션 코드에서는 극히 드문 케이스이므로 허용 범위 내입니다.

#27 — middleware.go / middleware_internal_test.go
ok/!ok 패턴 + 명시적 메시지, 정확한 구현입니다. package http 내부 테스트로 unexported 함수를 직접 검증한 방식이 깔끔합니다.

#37 — main.go / main_test.go
시그니처 변경 및 호출부 에러 처리 모두 정확합니다. def := DefaultRelayWorkerConfig() 제거는 DefaultRetryCount가 원래도 직접 할당이었고 withDefaults()에서 0값 처리를 하므로 문제없습니다. 테스트 3종(ValidDurations / InvalidRetryDelay / InvalidPollBackoff)이 단일 동작을 명확하게 검증합니다.

비블로커 관찰사항

  • DefaultRetryCount=0 케이스 테스트는 없으나 withDefaults()와 Viper 기본값이 보장하므로 생략 수준 판단은 적절합니다.

LGTM ✅

@ppzxc ppzxc merged commit a0068ca into main Mar 24, 2026
@ppzxc ppzxc deleted the issue/fix-27-30-37 branch March 24, 2026 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant