코드리뷰의 A-Z: Pull Request에서 Code Review까지
1. Git을 이용한 협업
1.1 각 브랜치의 역할
운영은 master, 새로운 기능 추가는 dev !
master
최종 검수가 완료된 코드 = 개발팀에서 운영서버에 배포해도 좋다고 합의가 이루어짐.
dev
기능 하나를 구현한 것. 보통 여러개의 commit으로 이루어짐.
<예제>
커밋 1. Host 리스트 조회 api 추가
커밋 2. Host Group 조회 api 추가
커밋 3. 호스트 조회 페이지 추가
1.2 협업시 코드의 품질을 높이는 방법
1.2.1 코드의 품질이 떨어진다는건 무슨 말일까?
여러 사람이 함께 일하다보면,
> 다른 사람이 새롭게 만든 기능의 존재조차 모를 경우가 있습니다. (내 업무 끝내기도 바쁜 상황)
> 나와 팀원의 코드가 서로 상충하여 충돌이 납니다.
> 주니어의 경우 정책에 어긋나는 코드를 만들기도 하죠.
> 코드의 가독성이 떨이지는 경우도 있구요. (자바스크립트 함수의 예제. var a,b,c,d, .....)
1.2.2 어떻게 해결할까?
Pull Request를 통해 팀원의 코드 리뷰를 받는다.
2. Pull Request
2.1 의미
"제가 새로운 기능을 추가했습니다. 제 브랜치를 받으셔서(Fetch) 검토후, master에 병합(merge)해주세요!"
3. Pull Request ~ Code Review 진행 방법
<예시>
개발자 Bob은 새로운 모니터링 기능을 추가했습니다.
운영에 배포 전, 사수 Song과 팀장 Lee에게 코드 리뷰를 받고자 합니다.
3.0 그림으로 보는 Pull Request 과정
3.1 개발자(Bob)가 Pull Request를 리뷰어에게 보냄
Step1. Master에서 새 브랜치 생성.
Step2. commit 후 remote에 push
Step3. github(https://github.com/)에서 pull request하기
* intelliJ에서도 진행할 수 있지만, 본 포스팅에서는 github에서 진행하는 방법만 설명합니다.
Step 4. Pull Request 상세 내역 작성
위 페이지에서 (1)~(5)의 의미를 살펴보자.
1) Pull request 보낼 브랜치 선택
(운영 branch) <- (내 dev)
2) Pull Request 제목 및 개발 내용 상세 설명.
(3)에서 지정될 리뷰어는 개발 내용에 대해 자세히 알지 못합니다.
따라서 코드의 전반 로직을 이해하고 리뷰를 시작할 수 있도록 친절하게 설명해줍시다.
3) Reviewer 설정(★중요★)
내 코드를 리뷰해줄 사람을 지정합니다.
** 타 개발팀에서는
리뷰어중 일정 수가 Pull Request를 Approve하지 않으면 운영 소스에 merge하지 않는다는 원칙을 가지기도 함.
4) Asignees
> 해당 Pull Request의 피드백을 바탕으로 코드를 수정할 사람.
> 팀에 따라 QA를 포함하기도 함.
(참고: https://newbedev.com/on-github-what-s-the-difference-between-reviewer-and-assignee_
5) Label
Pull Request의 카테고리입니다.
버그 수정, 문서 추가, 새로운 기능 추가 등 다양한 카테고리가 있습니다.
3.2 리뷰어의 Code Review 진행
Step1. GitHub>Pull Requests로 이동. 본인에게 할당된 PR 확인
Step2. Commits에서 추가된 기능들을 확인, Files Chaned에서 리뷰를 시작합니다.
Step3. Files Chaned에서 +를 누르고 리뷰를 시작합니다.
주의! Start a review 와 Add single comment 는 무엇이 다를까?
Start a review
> 여러 코멘트를 남길 때 사용.
: 본 버튼 클릭 후 화면 상단에 Review Changes 버튼이 활성화 되는 것을 볼 수 있음.
> Finish your review를 누를때까지 담당자에게 alert 가지 않음.
Add Single comment
> PR 요청자에게 바로 알람 감.
참고. (내가 생각하는) 좋은 리뷰
1. 수정이 필요한 이유를 명확히 설명함
2. 또 다른 대안, 더 나은 방법이 있다면 제안합니다.
3. 3 리뷰어가 Merge가능 여부 결정 (Finish your review)
- Comment
> Merge 가능 여부는 남기지 않고, 코드에 대한 의견만 남깁니다.
- Approve
> Merge 를 승인합니다. 이때 리뷰로 달린 코멘트는 권장사항이며, 당장에 적용은 필요가 없다고 여겨집니다.
- Request Changes.
> Merge 이전 수정이 필요할 때 씁니다.
3.4 개발자는 Review 결과에 따라 수정 진행.
3.5 최종 Approval을 거쳐 Master에 Merge
참고: Merge 담당자는 팀 내부 규약에 따라 다릅니다.
> Merge 담당자가 있는 경우도 있고,
> 해당 기능 개발자가 approval의 수를 체크한 후 직접 merge를 진행하기도 합니다.
주의! PR을 Master로 Merge시 선택사항
- Squash and Merge
dev 브랜치에서 여러개로 나눠졌던 commit을 하나의 커밋메시지로 묶어서 master에 병합합니다.
PM에게는 각각의 commit이 중요한 것이 아니라, 하나의 기능이 완성되었다는 사실이 중요하므로,
주로 이 옵션을 사용합니다.
- Merge
dev 브랜치의 commit 기록을 유지하고자 할때 사용합니다.
3.6 dev 브랜치를 삭제합니다.
4. 마치며: PR을 하는 우리의 자세
PR의 목적은 코드의 품질을 개선하는데 있습니다.
다른 개발팀에서는 어떤 코드 리뷰 문화를 가지고 있을까요?
리뷰어
1. 너무 세세한 피드백보다는 전체 구조, 가독성에 대해 리뷰해줍니다.
(예. 코드의 중복, 가독성등)
2. '~해라'가 아닌 '~하면 어떨까?' 라는 제안
부드러운 커뮤니케이션이 필요합니다.
3. 빠른 push 를 위해 서비스를 운영하는데 당장에 문제가 없는 부분은 넘어갑니다.
(너무 많은 request change는 운영의 속도를 저하시킵니다)
기능 개발자
1. 본인의 코드에 대해 최대한 간결하고 명확하게 설명합니다.
리뷰어는 바쁜 와중에 시간 내어 내 코드를 확인해줍니다. 따라서 로직에 대해 제대로 알려줘야합니다.
2. 리뷰를 부탁하기전 충분한 테스트를 거칩시다.
3. 테스트 코드를 작성하여 각 메소드의 의도를 드러냅니다.
때로는 README를 읽어도 명확하지 않은 기능들이 있습니다.
테스트 코드를 통해 input, output의 예시를 준다면 코드에 대한 이해도는 올라갈 것입니다.