Conversation
Test Results130 tests 130 ✅ 8s ⏱️ Results for commit 4a2d3da. ♻️ This comment has been updated with latest results. |
This2sho
force-pushed
the
Refactor/#66_스케줄러_모듈_분리
branch
from
aecaa1e to
b9929d2
Compare
| id: check_changes | ||
| run: | | ||
| git fetch origin main | ||
| if git diff --name-only origin/main...HEAD | grep -q "^domain/"; then |
There was a problem hiding this comment.
if 분기를 하니 ci flow 가 복잡한 느낌이 조금 드네요.
모듈 별로 ci 파일을 분기하는건 어떠신가요?
branches 트리거 부분을 paths 로 대체하여 "domain/**" 이런식으로 작성하면 해당 모듈에 변경이 발생하면 ci 가 동작하더라구요
There was a problem hiding this comment.
트리거 부분 예시입니다
on:
push:
paths:
- "domain/**" ```
There was a problem hiding this comment.
저도 처음에 path 로 트리거를 하고 모듈 별로 ci를 따로 하려고 했는데
그러면 다음과 같은 문제가 생깁니다.
- 테스트 수행을 제외한 이외의
step이 동일한데, 중복이 발생한다. - 테스트가 중복된다. (Domain모듈이 변경되는 경우, Domain을 의존하는 API, Scheduler가 같이 테스트 되어야하는데 path로 트리거를 하고 Domain 및 API 가 변경되면 Domain에서도 API를 테스트하고, API 모듈에서도 API를 테스트하게 됩니다)
- 테스트 결과를 모아서 볼 수가 없다.
app-api/src/main/java/com/parkingcomestrue/parking/application/exception/ClientException.java
Outdated
Show resolved
Hide resolved
app-api/src/test/java/com/parkingcomestrue/parking/application/config/TestConfig.java
Outdated
Show resolved
Hide resolved
app-api/src/test/java/com/parkingcomestrue/parking/application/container/ContainerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
이건 external 패키지 아래에 있는 클래스 전부에 대한 내용입니다.
모든 패키지가 external 패키지 하위에 있어서 external 패키지가 없어도 괜찮을 거 같아요.
뎁스만 하나 추가된 느낌?
There was a problem hiding this comment.
스프링 패키지 구조에서 com-회사 도메인-프로젝트명 이 관례로 알고 있습니다.
여기서 exteranl은 프로젝트 명으로 사용했는데, 다른 네이밍 있으면 추천해주세여
추가로 scanBasePackages로 하고 있다가 우형 멀티모듈 보고
Application을 상위 패키지(parkingcomestrue)로 올리는게 조금 더 확장성 있다고 생각해서 바꿨습니다.
| private final CoordinateService coordinateService; | ||
| private final ParkingService parkingService; | ||
| private final CoordinateApiService coordinateApiService; | ||
| private final ParkingRepository parkingRepository; | ||
|
|
||
| @Scheduled(cron = "0 */30 * * * *") | ||
| public void autoUpdateOfferCurrentParking() { |
There was a problem hiding this comment.
저 이거 이제 봤는데 혹시 @transactional 일부러 안붙인건가요??
There was a problem hiding this comment.
ㅋ_ㅋ 피드백 받고 #31 에서 뺐습니다.
There was a problem hiding this comment.
저 코멘트는 트랜잭션 범위에서 외부 api 통신하는 코드를 빼는 의도였는데 변경된 부분을 제가 리뷰하면서 놓쳤나봐요.
autoUpdateOfferCurrentParking 메서드 전체를 트랜잭션으로 묶는게 아니라 autoUpdateIfferCurrentParking 메서드의 readBy 아랫부분을 트랜잭션 범위로 묶는게 좋아보여요.
현재는 readBy 로 조회한 주차장 별로 커넥션 획득 -> 커밋 -> 해제가 발생할거같아요
이 부분은 모듈 분리랑은 거리가 있으니 이 PR 머지되고 수정해도 될 것 같아요.
There was a problem hiding this comment.
전체 주차장 조회를 하나의 트랜잭션으로 묶으란 말씀인가여?
👍관련 이슈
🤔세부 내용
도메인 모듈 분리 (도메인, infra(DB 관련))
API 모듈 분리 (컨트롤러, 어플리케이션 서비스 및 인증 관련)
스케줄러 모듈 분리
CI 스크립트 수정
CD 스크립트 수정
서버내 배포 스크립트 수정
서버내 docker-compose 수정
Exception� 모듈 의존성에 맞게 수정
🫵리뷰 중점사항