코드 리뷰 지침
이 가이드에는 코드 리뷰를 수행하고 코드를 검토하는 데 도움이 되는 조언과 모범 사례가 포함되어 있습니다.
GitLab CE 및 EE에 대한 모든 Merge Request은 GitLab 팀 멤버나 보다 넓은 커뮤니티 멤버가 작성한 것이든 효과적이고 이해하기 쉽고 유지보수 가능하며 안전한 코드를 보장하기 위해 코드 리뷰 프로세스를 거쳐야 합니다.
Merge Request이 검토되고 승인되어 Merge되는 과정
시작하기 전에:
- 기여 수락 기준을 숙지하세요.
- 도움이 필요한 경우(예: 첫 번째 Merge Request인 경우) Merge Request 코치 중 한 명에게 질문하십시오.
코드를 검토할 코드가 준비되면 검토자에 의해 코드를 검토받아야 합니다. 검토자는 귀하의 그룹 또는 팀에서거나 도메인 전문가에서 선택할 수 있습니다. 검토자는 다음을 할 수 있습니다:
- 선택한 솔루션 및 구현에 대해 두 번째 의견을 제시합니다.
- 버그, 논리 문제 또는 해결되지 않은 예외 상황을 찾는 데 도움을 줍니다.
Merge Request이 작고 쉽게 검토할 수 있는 경우 검토자 단계를 건너뛸 수 있으며 직접 유지 관리자에게 질문할 수 있습니다.
무엇이 “작고 간단한” 것인지는 모호한 영역입니다. 여기 작고 간단한 변경 사항의 예가 있습니다:
- 오탈자 수정이나 작은 문구 변경 (예시).
- 어떠한 행동이나 데이터를 변경하지 않는 작은 리팩터링.
- 1개월 이상 기본으로 활성화된 피처 플래그에 대한 참조 제거.
- 사용되지 않는 메서드 또는 클래스 제거.
- < 5줄의 코드를 변경해야 하는 잘 이해된 논리 변경사항.
그렇지 않으면, 유지 관리자가 Merge Request을 먼저 검토해야하며 MR이 건드리는 각 카테고리(예: 백엔드, 데이터베이스)에서 검토자가 검토해야 합니다. 유지 관리자가 관련 도메인 지식을 가지고 있지 않을 수 있으므로 이것은 작업 부하를 분산시키는 데도 도움이 됩니다.
보안 스캔 또는 코멘트를 처리하는 데 도움이 필요한 경우, Application Security Team (@gitlab-com/gl-security/appsec
)를 포함하십시오.
검토자들은 사이드바에 있는 검토 기능을 사용합니다. 검토자는 추가적으로 승인함으로써 승인을 추가할 수 있습니다.
Merge Request이 건드리는 영역에 따라 하나 이상의 유지 관리자에 의해 승인되어야 합니다. Approved 버튼은 Merge Request 위젯에 있습니다.
Merge Request을 Merge 받으려면 또 다른 유지 관리자가 필요합니다. 1개 이상의 승인이 필요한 경우, Merge을 검토하고 승인하는 마지막 유지 관리자가 Merge합니다.
일부 도메인 영역(Verify
와 같은 영역)은 CODEOWNERS 규칙에 따라 도메인 전문가의 승인이 필요합니다. CODEOWNERS 섹션은 독립적인 승인 규칙이며, 더 일반적인 승인 규칙(for example 백엔드
)의 하위 집합이 될 수 있는 특정 규칙(예: Verify
)이 있을 수 있습니다. 더 효율적인 프로세스를 위해 작성자는 일반적인 승인 전에 도메인별 승인을 찾아야 합니다.
도메인별 승인자도 유지 관리자일 수도 있으며, 그렇다면 도메인 특이 사항과 보다 넓은 변경 사항을 동시에 검토하고 모든 역할에 대해 한 번 승인해야 합니다.
자세한 내용은 아래의 작성자 책임을 참조하세요.
도메인 전문가
도메인 전문가는 특정 기술, 제품 기능 또는 코드베이스 영역에 상당한 경험을 가진 팀 멤버입니다. 팀 멤버는 도메인 전문가로서 자체 식별하는 것을 장려받으며 팀 프로필에 추가해야 합니다.
도메인 전문가로 자체 식별할 때, .yml
파일을 변경하는 MR을 이미 확립된 도메인 전문가나 해당하는 엔지니어링 매니저에 의해 Merge되도록 할 것을 권장합니다.
다음과 같은 가정을 하고 있습니다.
- 특정 단계/그룹(예: create: source code)에서 작업하는 팀 멤버는 해당 앱 영역의 도메인 전문가로 간주됩니다.
- 특정 기능(예: 검색)에서 작업하는 팀 멤버는 해당 기능에 대한 도메인 전문가로 간주됩니다.
우리는 기본적으로 코드 리뷰를 위한 도메인 전문가인 팀 멤버에게 리뷰를 지정합니다. 디자인 리뷰는 디자이너 용량 한계로 인해 상품 디자이너가 지원하지 않는 영역은 커뮤니티 기여일 경우를 제외하고 UX 리뷰가 더 이상 필요하지 않습니다. 도메인 전문가가 적합하지 않은 경우, 팀 멤버 중 아무나를 선택하여 MR을 검토하거나 검토자 룰렛 권장 사항을 따를 수 있습니다.
도메인 전문가를 찾기 위해:
- Merge Request 승인 위젯에서 View eligible approvers를 선택하세요. 이 위젯은 코드베이스 영역별로 권장 및 필요한 승인을 표시합니다. 이러한 규칙은 Code Owners에서 정의됩니다.
- Merge Request과 관련된 단계 또는 그룹에 작업하는 팀 멤버의 디렉터리을 확인하세요.
- 공학 프로젝트 페이지나 GitLab 팀 페이지에서 팀 멤버의 도메인 전문성을 확인하세요. 도메인은 자체 식별되므로 귀하의 판단을 사용하여 MR의 변경 사항을 도메인에 매핑하세요.
- MR에 기여한 파일을 확인하는 팀 멤버를 찾으세요.
git log <file>
을 실행하여 로그를 볼 수 있습니다. - 파일을 검토한 팀 멤버를 찾으세요.
git log <file>
을 사용하여 관련 Merge Request을 찾을 수 있습니다.-
git log <file>
을 사용하여 커밋 SHA를 얻으세요. -
<SHA>
를 사용하여https://gitlab.com/gitlab-org/gitlab/-/commit/
로 이동하세요. - 커밋에 표시된 관련 Merge Request을 선택하세요.
-
리뷰어 룰렛
위험 봇은 당신의 Merge Request이 영향을 미칠 것으로 보이는 코드베이스 각 영역에 대해 리뷰어와 메인테이너를 무작위로 선택합니다. 개발자 리뷰어를 위한 권장 사항을 제시하며, 누군가가 더 적합하다고 생각되면 이를 재정의해야 합니다.
승인 가이드라인은 도메인 전문가를 선택하는 데 도움이 될 수 있습니다.
우리는 제품 디자이너를 포함하는 팀의 MR에 대해서만 UX 리뷰를 실시합니다. 이러한 팀의 사용자를 대상으로 하는 변경 사항은 피처 플래그에 숨겨져 있더라도 UX 리뷰가 필요합니다. 권장하는 UX 리뷰어를 기본값으로 설정하십시오.
엔지니어링 프로젝트 페이지의 디렉터리에서 리뷰어와 메인테이너를 선택하는 행동은 다음과 같습니다:
- 그들이 Slack 또는 GitLab 상태에 다음을 포함하는 사람은 선택되지 않습니다:
-
OOO
,PTO
,출산 휴가
,친구 및 가족
, 또는컨퍼런스
문자열이 포함된 경우 - GitLab 사용자 바쁨 표시가
True
로 설정된 경우 - 이모지가 다음 카테고리 중 하나인 경우:
-
휴가 중 - 🌴
:palm_tree:
, 🏖️:beach:
, ⛱:beach_umbrella:
, 🏖:beach_with_umbrella:
, 🌞:sun_with_face:
, 🎡:ferris_wheel:
, 🏙:cityscape:
-
병가 중 - 🌡️
:thermometer:
, 🤒:face_with_thermometer:
-
용량 초과 - 🔴
:red_circle:
-
집중 모드 - 💡
:bulb:
(팀의 작업에 집중함)
-
휴가 중 - 🌴
-
- 이미 선택된 리뷰 수가 선택한 “리뷰 제한”과 동일하거나 그보다 많은 사람은 선택되지 않습니다. 리뷰 제한은 한 번에 처리할 수 있는 리뷰의 최대 수입니다. 리뷰 제한은 다음 옵션 중 하나를 Slack 또는 GitLab 상태로 설정하여 설정합니다:
- 0️⃣ -
:zero:
(:red_circle:
와 유사함) - 1️⃣ -
:one:
- 2️⃣ -
:two:
- 3️⃣ -
:three:
- 4️⃣ -
:four:
- 5️⃣ -
:five:
보안 그룹의 기본 브랜치를 대상으로 하지 않는 Merge Request의 리뷰 요청은 계산되지 않습니다. 이러한 MR은 일반적으로 백포트이며, 유지자 또는 리뷰어는 보통 이를 리뷰하는 데 많은 시간이 필요하지 않습니다.
- 0️⃣ -
- 리뷰에 대한 허기: Slack 또는 GitLab 상태 이모지가 🔵
:large_blue_circle:
인 팀 구성원들이 더 자주 선택됩니다. 이것은 리뷰어와 수습 유지자에게 적용됩니다.- 🔵
:large_blue_circle:
리뷰어는 다른 리뷰어보다 2배 더 자주 선택됩니다. - 🔵
:large_blue_circle:
수습 유지자는 다른 리뷰어보다 3배 더 자주 선택됩니다.
- 🔵
-
GitLab 상태 이모지가 🔶
:large_orange_diamond:
또는 🔸:small_orange_diamond:
인 사람은 선택될 가능성이 절반으로 줄어듭니다. - 이전과 동일한 브랜치 이름에 대해 항상 같은 리뷰어와 유지자를 선택합니다(OOO 상태가 변경되지 않는 한). 이를 통해 백포트 브랜치에 대해 안정적으로 선택됩니다.
-
GitLab 상태 이모지가 Ⓜ
:m:
인 사람은 유지자인 프로젝트에서만 리뷰어로 제안됩니다.
룰렛 대시보드에는 다음이 포함되어 있습니다:
- 지난 7일 및 30일의 할당 이벤트.
- 각 사람 당 현재 할당된 Merge Request.
- 다양한 기준으로 정렬.
- 매뉴얼 리뷰어 룰렛.
- 현지 시간 정보.
더 많은 정보를 원하시면 룰렛 README를 확인하세요.
승인 지침
유지자의 책임에 관한 섹션에서 설명한 대로, 당신은 Merge Request을 도메인 전문가로부터 승인 및 Merge할 것을 권장받습니다. 첫 번째 리뷰어의 선택 사항에 대해 여기에서 다루지 않습니다. 그러나 Merge Request은 개요 섹션에서 설명한 대로 리뷰어에게 검토되어야 합니다.
Merge Request에 포함된 내용 | 승인이 필요한 대상 |
---|---|
~backend 변경 사항 1
| Backend 유지자. |
~database 마이그레이션 또는 비용이 큰 쿼리 변경 2
| Database 유지자. 자세한 내용은 데이터베이스 리뷰 가이드라인을 참조하세요. |
~workhorse 변경 사항
| Workhorse 유지자. |
~frontend 변경 사항 1
| Frontend 유지자. |
~UX 사용자 중심 변경 사항 3
| 제품 디자이너. 자세한 내용은 디자인 및 사용자 인터페이스 가이드라인을 참조하세요. |
새로운 JavaScript 라이브러리 추가 1 | - 프론트엔드 foundations 구성원 (라이브러리가 번들 크기를 크게 증가시킬 경우) - GitLab에서 승인되지 않은 라이선스를 사용하는 경우 법률 부서 구성원 (GitLab 라이선스 및 호환성 문서에서 라이선스 호환성에 대한 자세한 내용을 확인할 수 있습니다(licensing.md)). |
새로운 의존성 또는 파일 시스템 변경 | - 배포 팀 구성원. 자세한 내용은 배포 팀과 협업하는 방법을 참조하세요. - RubyGems의 경우 AppSec 리뷰 신청. |
~documentation 또는 ~UI 텍스트 변경
| 적절한 DevOps 스테이지 그룹의 할당을 기반으로 기술 작가. |
개발 지침 변경 | 검토 프로세스를 따르고 그에 따라 승인을 얻으세요. |
그리고 비앤드-투-비앤드 변경 4 | 테스트 개발자. |
오직 비앤드-투-비앤드 변경 4 또는 MR 작성자가 테스트 개발자일 경우 | 품질 유지자. |
GitLab에 새로운 서비스 추가(예: Puma, Sidekiq, Gitaly) | 제품 관리자. 자세한 내용은 GitLab에 서비스 컴포넌트 추가 프로세스를 참조하세요. |
인증 관련 변경 |
관리:인증. 자세한 내용은 그룹 페이지의 코드 리뷰 섹션을 확인하세요. 해당 팀에 대해 리뷰가 필요한 파일의 패턴은 CODEOWNERS 파일의 인증 섹션에 나열되어 있으며, 수정한 모든 Merge Request의 승인자 섹션에 해당 팀이 나열됩니다.
|
사용자 정의 역할 또는 정책과 관련된 변경 | 관리:인가 엔지니어. |
CODEOWNERS 승인
일부 Merge Request은 특정 그룹의 필수 승인이 필요합니다.
정의에 대해서는 .gitlab/CODEOWNERS
를 참조하세요.
.gitlab/CODEOWNERS
에서 필수 섹션은 다음과 같은 경우에만 제한되어야 합니다:
- 규정 준수
- 가용성
- 보안
필수 섹션을 추가할 때, Merge Request 비율에 미치는 영향을 추적해야 합니다. 좋은 예제는 검증 이슈를 참조하세요.
모든 다른 경우에는 우리는 강도보다는 책임성을 우선시하기 때문에 필수 섹션을 사용해서는 안됩니다.
또한, 모놀리식의 현재 구조는 Merge Request이 보이지 않는 부분에 접근할 가능성이 높습니다. 여러 필수 승인이 있으면 이러한 Merge Request에 대한 작성자의 승인을 요청해야 하며, 이는 효율적이지 않습니다.
이에 대한 개선 노력은 다음과 같습니다:
- https://gitlab.com/groups/gitlab-org/-/epics/11624
- https://gitlab.com/gitlab-org/gitlab/-/issues/377326
수락 점검 디렉터리
이 체크리스트는 Merge Request(MR)의 작성자, 리뷰어 및 유지 관리자가 변경 사항을 품질, 성능, 신뢰성, 보안, 관측 가능성 및 유지 관리성에 대한 고위험 위험에 대해 분석했음을 확인하는 데 도움을 주는 것입니다.
체크리스트 사용은 소프트웨어 엔지니어링의 품질을 향상시킵니다. 본 체크리스트는 GitLab 코드베이스의 기여자들의 능력을 지원하고 강화하는 간단한 도구입니다.
품질
자세한 품질 가이드 라인에 대해서는 테스트 엔지니어링 프로세스를 참조하세요.
- 이 MR을 코드 검토 지침에 따라 자체 검토했습니까?
- 이 변경 사항에 영향을 미치는 코드에 대한 자동화된 테스트(Testing Guide)가 사용자에게 매우 중요한 기능을 확인한다고 믿습니까?(테스트 수준testing levels을 고려하여)
- 기존의 자동화된 테스트가 위의 기능을 포함하지 않는 경우, 필요한 추가 테스트를 추가했거나 이에 대한 자동화 테스트 갭에 대한 문제를 추가하고 MR에 연결했습니까?
- 이 변경 사항이 GitLab.com 호스팅 고객 및 Self-managed 고객에 미치는 기술적 측면을 고려했습니까?
- 이 변경 사항이 시스템의 프론트엔드, 백엔드 및 데이터베이스 부분에 미치는 영향을 적절하게 고려하고
~ux
,~frontend
,~backend
,~database
라벨을 적용했습니까? - 이 MR을 지원되는 모든 브라우저에서 테스트했거나, 이러한 테스트가 필요하지 않다고 결정했습니까?
- 이 변경 사항이 업데이트를 통해 하위 호환성이 있는지 확인했거나, 이것이 적용되지 않았다고 결정했습니까?
- EE(Enterprise Edition)의 내용을 적절하게 분리했거나, 이 MR이 FOSS(Free and Open Source Software) 전용인지 확인했습니까?
- EE 코드는 어디에 두어야 하나요?](ee_features.md)
- 이 MR이 EE 및 FOSS에 각기 다른 방식으로 영향을 줄 수 있다면, FOSS 컨텍스트에서 CI 파이프라인을 실행하는 것을 고려했습니까?
- 기존 데이터가 놀랍게 다양할 수 있음을 고려했습니까? 예를 들어, 새로운 모델 유효성 검사가 기존 레코드를 손상시킬 수 있습니다. 기존 데이터가 유효성 검사를 통과할 것이라고 확신하지 않았다면, 기존 데이터에 대한 검사를 필수가 아닌 선택 사항으로 만드는 것을 고려했습니까?
- 테스트가 경고와 함께 통과하고 실패한 작업에
Flaky test '<path/to/test>' was found in the list of files changed by this MR.
라는 텍스트가 포함된 경우, 이 테스트를 수정하거나 테스트가 왜 무시될 수 있는지 설명하는 증거를 제공했습니까?
성능, 신뢰성 및 가용성
- 이 MR에 의해 성능이 저하되지 않을 것이라고 확신하거나 리뷰어에게 성능 영향을 평가해 달라고 요청하였나요? (Merge Request 성능 가이드 라인)
- MR 설명에 데이터베이스 관련 변경 사항에 대한 정보 추가를 했거나 필요하지 않다고 결정하였나요?
- 이 변경에 따른 가용성 및 신뢰성 리스크를 고려했습니까?
- 예상된 성장을 기반으로 한 확장 가능성의 위험을 고려했습니까?
- 이 변경이 평균 고객보다 훨씬 더 많은 데이터를 가진 대형 고객에게 미치는 성능, 신뢰성 및 가용성 영향을 고려했습니까?
- 이 변경이 최소 시스템에서 GitLab을 실행할 수 있는 고객에게 미치는 성능, 신뢰성 및 가용성 영향을 고려했습니까?
관측 가능성 계측
- 관측 가능성을 통해 디버깅 및 적극적인 성능 향상을 위한 충분한 계측을 포함했습니까? 예시를 참조하여 특성 플래그, 로깅 및 계측 추가.
문서
- 변경 사항에 대한 변경 로그 트레일러를 포함했거나 이것이 필요 없다고 결정하였나요?
- 문서를 추가/업데이트했거나 이 변경 사항에 대해 문서 변경이 필요하지 않다고 결정하였나요?
보안
- 이 MR에 자격 증명 또는 토큰 처리 또는 저장, 인증 방법 또는 다른 보안 검토 가이드 라인에 설명된 다른 항목에 변경 사항이 포함되어 있다면,
~security
라벨을 추가하고@gitlab-com/gl-security/appsec
를 언급하였나요? - 언제와 어떻게 내부 애플리케이션 보안 검토에 대한 문서를 검토하고,이 변경 사항에 대해 보안 검토가 필요하다면 보안 검토를 요청하였나요?
- MR을 차단하는 보안 스캔 결과가 있는 경우(Merge Request 승인 정책으로 인해):
- 실제 양성 결과의 경우, 이를 해결한 후 MR을 Merge하기 전에 부적절한 애플리케이션 보안 승인을 제거합니다.
- 가짜 양성 결과, 위험 수용에 대한 토론이 필요한 경우 또는 의문 사항이 있는 경우,
@gitlab-com/gl-security/appsec
을 언급하세요.
배포
- 변경 사항이 높은 위험이 될 수 있기 때문에이 변경에 대한 피처 플래그 사용을 고려했습니다.
- 피처 플래그를 사용하는 경우, 프로덕션에서 테스트하기 전에 스테이징에서 변경 사항을 테스트할 계획이며 모든 고객에게 전체적으로 적용하기 전에 일부 프로덕션 고객에게 전개하는 것을 고려해보았습니다.
- 완료 정의, 또는 이것이 불필요하다고 판단하여 인프라 부서에 기본 설정 또는 새로운 설정 변경에 대해 알렸습니다.
규정 준수
- 올바른 MR 유형 라벨이 적용되었는지 확인했습니다.
합병 요청 작성자의 책임
최적의 해결책을 찾고 구현하는 책임은 합병 요청 작성자에게 있습니다. 작성자 또는 직접적으로 책임 있는 개인 (DRI)은 코드 검토 라이프사이클 전체 기간 동안 합병 요청의 담당자로 지정되어 있습니다. 자신을 담당자로 설정할 수없는 경우 리뷰어에게 요청하여 대신해야 합니다.
유지보수자에게 승인 및 합병을 요청하기 전에,
- 그것이 의도한 문제를 실제로 해결하는지
- 가장 적합한 방법으로 그것을 수행하는지
- 모든 요구 사항을 충족하는지
- 남아있는 버그, 논리적 문제, 처리되지 않은 엣지 케이스 또는 알려진 취약점이 있는지 여부를 확인해야 합니다.
이것을 신뢰할 수 있도록 하는 가장 좋은 방법은 리뷰어와의 불필요한 양방향 소통을 피하기 위해 자체 합병 요청의 검토를 수행하는 것입니다. 이 자체 검토 과정에서, 의사 결정 또는 트레이드 오프가 이루어진 부분이나 상황에 대한 컨텍스트 설명이 리뷰어가 코드를 더 쉽게 이해하는 데 도움이 될 수 있는 행에 주석을 포함해보세요.
해당 솔루션에 필요한 수준의 신뢰를 얻기 위해, 작성자는 조사 및 구현 프로세스에 적절한 사람들을 참여시킬 것으로 예상됩니다.
이들은 서로 다른 솔루션을 논의하기 위해 도메인 전문가에게 문의하거나 구현을 검토하기 위해 제품 관리자 및 UX 디자이너에게 혼란을 해소하거나 최종 결과가 그들이 생각한 것과 일치하는지 확인하기 위해 연락할 것을 권장받습니다. 데이터 모델 또는 특정 쿼리에 대한 입력을 얻기 위해 데이터베이스 전문가에게 문의하거나 다른 개발자에게 솔루션에 대한 깊은 리뷰를 받을 수 있습니다.
기능을 제공하려면 여러 개의 합병 요청이 필요할 것으로 예상되는 경우 (예: 증명 사례를 만들었으며 기능이 10개 이상의 합병 요청으로 구성될 것으로 명확한 경우), 해당 기능을 이해하는 데 필요한 리뷰어 및 유지보수자를 식별할 것을 고려해보세요 (그들과 컨텍스트를 공유할 것). 그런 다음 모든 합병 요청을 이러한 리뷰어에게 직접 지시하세요. 이러한 리뷰어를 찾는 데 가장 좋은 DRI는 EM 또는 스탭 엔지니어입니다. 동일한 컨텍스트의 여러 합병 요청에 대한 안정적인 리뷰어 상대로는 효율성을 높입니다.
합병 요청이 여러 도메인에 영향을 미칠 경우 (예: Dynamic Analysis와 GraphQL), 각 도메인에서 전문가로부터 리뷰를 요청하세요.
작성자가 도메인 전문가의 의견이 필요한지 확신이 들지 않는다면, 그것이 필요하다는 것을 나타냅니다. 그것이 없으면 그들은 솔루션에 필요한 수준의 자신감을 가질 가능성이 적습니다.
검토하기 전에, 작성자는 합병 요청 차이에 대한 주요 사항을 알릴 주석을 제출하도록 요청되었습니다. 또한 더 많은 설명 또는 주의를 요구하는 경우를 대비해 주요 콘텐츠의 예시는 다음과 같습니다:
- 린팅 규칙의 추가 (RuboCop, JS 등).
- 라이브러리의 추가 (Ruby gem, JS lib 등).
- 명확하지 않은 경우, 상위 클래스 또는 메소드에 대한 링크.
- 변경을 보완하기 위해 수행된 모든 벤치마킹.
- 잠재적으로 보안 취약한 코드.
해당 솔루션을 확인하기 위해 검토자에게 필요한 프로젝트, 스니펫 또는 기타 에셋이 있는 경우, 검토자가 해당 에셋에 액세스 할 수 있도록해야합니다.
리뷰어를 할당 할 때, 다음 사항을 고려하는 것이 도움이 될 수 있습니다:
- 해당 리뷰어가 제공해야 하는 리뷰 유형을 MR에 대한 주석으로 추가합니다.
- 예를 들어, MR이 데이터베이스 쿼리를 변경하고 백엔드 코드를 업데이트하는 경우, MR 작성자는 먼저
~backend
리뷰 및~database
리뷰가 필요합니다. 리뷰어를 할당하는 동안 각 리뷰어에게 어떤 도메인을 리뷰해야 하는지 알려주는 MR에 주석을 추가합니다. - 많은 GitLab 팀 구성원들이 하나 이상의 영역에서 도메인 전문가이기 때문에 이러한 유형의 주석이 없으면 때로는 제공해야 할 리뷰 유형이 명확하지 않을 수도 있습니다.
- MR 리뷰 유형에 대한 명시성은 MR 작성자에게 효율적이며 요청한 리뷰 유형을 즉시 알고 제공해야 하기 때문에 MR 리뷰어에게 효과적입니다.
- 예시 1
- 예시 2
- 예를 들어, MR이 데이터베이스 쿼리를 변경하고 백엔드 코드를 업데이트하는 경우, MR 작성자는 먼저
피하는 것:
- (위에 언급된) TODO 코멘트를 소스 코드에 직접 추가하지 않습니다. 수정 가능한 작업으로 인해 TODO 코멘트가 추가 된 경우, 관련된 이슈에 대한 링크가 포함되어야합니다.
- 코드가 하는 것을 설명하는 데 사용되는 코멘트를 추가하지 않습니다. 코멘트가 추가 된 경우 그것이 왜 있는지가 설명되어야 합니다.
- 실패한 테스트가 있는 MR을 유지보수자 검토를 요청하지 않습니다. 테스트가 실패하고 리뷰를 요청해야하는 경우, 설명이 포함된 코멘트를 반드시 남겨야합니다.
- 이메일 또는 Slack을 통해 (유지보수자가 Slack을 통해 접근 가능한 경우) 유지보수자를 지나치게 언급하지 마세요. MR에 리뷰어를 추가할 수 없는 경우, 코멘트에서 유지보수자를 언급하는 것은 허용되며 다른 모든 경우에 리뷰어를 추가하는 것으로 충분합니다.
이로써 리뷰어의 시간을 절약하고 작성자가 실수를 빨리 찾을 수 있습니다.
리뷰어의 책임
리뷰어는 선택한 솔루션의 구체적인 부분을 검토하는 책임이 있습니다.
Merge Request을 검토하세요를 철저히 합니다.
Merge Request이 기여 수락 기준을 모두 충족하는지 확인하세요.
일부 Merge Request은 구체적인 내용에 대한 도메인 전문가의 도움이 필요할 수 있습니다. 도메인 전문가가 아닌 경우 리뷰어는 다음 중 하나를 수행할 수 있습니다:
- Merge Request을 검토하고 다른 리뷰어나 메인테이너로 루프를 돌려 도메인 전문가를 참여시킵니다. 이 전문가는 다른 리뷰어이거나 메인테이너일 수 있습니다.
- 더 적합한 리뷰어로 리뷰를 전달합니다.
- 도메인 전문가가 없는 경우, 최선을 다해 리뷰합니다.
작성자를 요청해서 Merge Request을 분할하도록 안내해야 합니다 만약 그 Merge Request이:
- 너무 큰 경우
- 하나 이상의 문제를 해결하는 경우
- 두 가지 이상의 기능을 구현하는 경우
- 추가적인 리스크를 야기하는 높은 복잡성을 가지는 경우
작성자는 현재의 메인테이너들과 리뷰어들이 분할된 MRs를 검토하거나 새로운 메인테이너들과 리뷰어들을 요청할 것입니다.
모든 요구 사항을 충족했다고 확신이 들 때 다음을 수행해야 합니다:
- 승인을 선택합니다.
- 작성자를
@
언급하여 할 일 알림을 생성하고, 그들의 Merge Request이 검토되어 승인되었음을 알립니다. - 메인테이너로 리뷰를 요청합니다. 도메인 전문가들 중에서 필요한 메인테이너에게 기본값을 요청하나, 해당 메인테이너가 없거나 Merge Request이 도메인 전문가의 리뷰가 필요하지 않다고 생각되면 리뷰어 룰렛 제안을 따르는 것도 자유롭습니다.
- 본인을 리뷰어에서 제거합니다.
메인테이너의 책임
메인테이너는 GitLab 코드베이스의 전반적인 건강, 품질 및 일관성에 책임이 있습니다.
그 결과, 그들의 리뷰는 주로 전체 아키텍처, 코드 구성, 관심사의 분리, 테스트, DRYness, 일관성 및 가독성과 같은 사항에 중점을 둡니다.
메인테이너의 작업은 특정 도메인에 대한 지식이 아닌 GitLab 코드베이스의 전반적인 지식에만 의존하므로 어떤 팀 및 제품 영역에서든 Merge Request을 검토, 승인 및 Merge할 수 있습니다.
메인테이너는 Merge Request의 수락 기준이 합당하게 충족되었는지 확인하는 책임이 있습니다. 일반적으로 품질은 모두의 책임이지만 MR의 메인테이너는 MR이 해당 일반적인 품질 기준을 충족하는지 확인하는 책임을 지게 됩니다.
이것은 후속 이슈에서 기술적 부채 생성을 피함을 포함합니다.
MR이 충분히 중요하다고 느낄 때 또는 도메인 전문가가 필요한 경우, 메인테이너는 다른 리뷰어 또는 메인테이너로부터 리뷰를 요청할 재량이 있습니다. 리뷰 중에 메인테이너가 이러한 사항을 선제적으로 처리하는 예시:
- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/82708#note_872325561
- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38003#note_387981596
- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/14017#note_178828088
메인테이너들은 Merge 전에 선택한 솔루션의 구체적인 부분을 검토하기 위해 최선을 다합니다만, 그들이 반드시 도메인 전문가가 아니기 때문에 무리한 시간 투자 없이는 이를 수행하기 어려울 수 있습니다. 이러한 경우에는 작성자와 이전 리뷰어들의 판단에 우선하여 기본 업무에 집중하기 위해 그것을 우선합니다.
메인테이너이자 개발자인 경우, 리뷰어로 참여했다면 최종 승인 및 Merge을 함께하는 메인테이너로 선택되지 않는 것이 권장됩니다.
Merge하기 전에 메인테이너는 필요한 승인자에 의해 Merge Request이 승인되었는지 확인해야 합니다. 아직 다른 사람의 추가적인 승인을 기다리고 있다면 리뷰어에서 본인을 제거한 후 작성자를 @
언급하고 이유를 설명하는 코멘트를 달아야 합니다. 코드를 Merge하는 경우에는 리뷰어로 남아 있어야 합니다.
특정 Merge Request은 안정된 브랜치를 타겟으로 할 수 있습니다. 이러한 요청을 처리하는 방법에 대한 개요는 패치 릴리스 런북를 참조하세요.
Merge 후에 메인테이너는 Merge Request에 명시된 리뷰어로 남아 있어야 합니다.
리뷰어 기능 도그푸딩
2021년 3월 18일에 갱신된 프로세스가 효율적이고 일관성 있게 리뷰어 기능의 도그푸딩을 목적으로 한 새로운 프로세스를 적용했습니다.
변경사항을 요약한 것은 아래에서 확인할 수 있습니다.
- Merge Request 작성자와 DRI는 담당자(Assignees)로 남아 있습니다.
- 작성자는 사용자를 리뷰어로 할당함으로써 리뷰를 요청합니다.
- 리뷰어는 자신의 리뷰를 마친 후에 자신을 언어송하는 것입니다.
- 마지막 승인자(MR을 Merge하는 사람)는 리뷰어로 남아 있습니다.
최상의 실천 방법
모두
- 친절하게 행동하세요.
- 많은 프로그래밍 결정이 의견입니다. 어떤 걸 선호하는지, 빨리 해결방안에 도달하세요.
- 질문하세요. 명령을 내리지 마세요. (“이 이름을
:user_id
로 하는 것에 대해서 어떻게 생각합니다?”) - 명확성을 요청하세요. (“이해하지 못했습니다. 명확하게 할 수 있나요?”)
- 코드에 대한 주도적인 소유를 피하세요. (“내 것”, “내 것이 아님”, “너의 것”)
- 개인적인 특성을 가리킬 수 있는 용어를 사용하지 마세요. (“멍청한”, “바보 같은”). 모두가 지적이고 선량한 사람으로 가정하세요.
- 명시적으로 표현하세요. 온라인에서 의도를 이해하지 못하는 경우가 있습니다.
- 겸손해지세요. (“확실하지 않아요 - 확인해 봅시다.”)
- 과장하지 마세요. (“항상”, “절대로”, “끝없이”, “아무것도”)
- 무례한 행동으로 받아들여질 수 있는 살친말 사용에 주의하세요. 우리의 모든 행동은 공개적입니다; 장시간 동료에게 좋은 의도로 보이는 것이 새로운 프로젝트에 참여하는 사람에게는 무례하고 환영받지 않는 것처럼 보일 수 있습니다.
- “이해하지 못했습니다” 또는 “대안 솔루션:”이라는 댓글이 너무 많은 경우 일대일 채팅이나 영상 통화를 고려하세요. 일대일 토론을 요약하는 댓글을 게시하세요.
- 특정 사람에게 질문을 할 때는 항상 그들을 언급하여 댓글을 시작하십시오. 그렇게 하면 그들의 알림 수준이 “언급”으로 설정되어 있으면 알림을 받을 수 있고 다른 사람은 응답할 필요가 없음을 이해할 수 있습니다.
Merge Request이 리뷰되는 방법
코드 검토는 여러 번의 반복을 거칠 수 있는 프로세스이며, 리뷰어는 후에 알아차리지 못했을 수 있는 것을 나중에 발견할 수 있습니다.
- 코드의 첫 번째 리뷰어는 당신 입니다. 새로운 브랜치를 푸시하기 전에 전체 차이를 읽어보세요. 이해되나요? 변경 내용의 전반적인 목적과 관련이 없는 것이 포함되지는 않았는지 확인하세요. 디버깅 코드를 제거하는 것을 깜빡한 건 없나요?
- Merge Request 가이드라인에 기술된 대로 자세한 설명을 작성하세요. 어떤 리뷰어는 제품 기능이나 코드베이스의 특정 영역에 익숙하지 않을 수 있습니다. 철저한 설명은 모든 리뷰어가 요청을 이해하고 효과적으로 테스트할 수 있도록 돕습니다.
- 만약 여러분의 변경사항이 먼저 Merge되어야 한다면 설명에 기록하고 Merge Request 의존성을 설정하세요.
- 리뷰어의 제언에 감사드리세요. (“좋은 제안입니다. 변경하겠습니다.”)
- 개인적으로 받아들이지 마세요. 리뷰는 코드를 대상으로 하는 것이지 여러분을 대상으로 하는 것은 아닙니다.
- 코드가 왜 존재하는지 설명하세요. (“이유 때문에 이렇게 되었습니다. 만약에 이 클래스/파일/메소드/변수를 다른 이름으로 바꾸면 더 명확할까요?”)
- 관련 없는 변경 사항과 리팩터링은 향후 Merge Request/이슈로 분리하세요.
- 리뷰어의 관점을 이해하려 노력하세요.
- 모든 코멘트에 응답하려 노력하세요.
- Merge Request 작성자는 완전히 해결한 스레드에 대해서만 해결해야 합니다. 여전히 열린 답글, 제안, 질문, 또는 기타 무엇인가가 있는 경우, 해당 스레드는 리뷰어가 해결하도록 남겨둬야 합니다.
- 모든 피드백이 반드시 요청된 변경 사항으로 Merge Request에 통합되어야 한다고 가정해서는 안 됩니다. 필요한 경우 이는 Merge되기 전에 해당 Merge Request이미지의 피드백을 다루기 위해 향후 이슈를 생성해야 하는가에 대한 작정을 하여야 합니다.
- 이전 라운드의 피드백에 기반한 커밋을 분리된 커밋으로 브랜치에 푸시하세요. 브랜치가 Merge될 준비가 될 때까지 squash하지 마세요. 리뷰어는 이전 피드백에 기반한 개별 업데이트를 읽을 수 있어야 합니다.
- 리뷰의 추가 라운드를 위해 준비가 되면 리뷰어에게 새로운 리뷰를 요청하세요. 리뷰를 요청할 수 없다면
@
를 사용하여 리뷰어를 언급하면 됩니다.
리뷰를 요청하는 방법
Merge Request을 리뷰할 준비가 되었을 때는 초기 리뷰를 요청하여 승인 가이드라인을 기반으로 리뷰어를 선택하세요.
Merge Request이 여러 영역을 리뷰해야 하는 경우, 어떤 영역을 리뷰어가 검토해야 하는지와 어느 단계에서(첫 번째 또는 두 번째) 리뷰어가 되어야 하는지를 명시하는 것이 좋습니다. 이는 여러 분야에 대해 리뷰어 자격이 있는 팀 멤버들이 어떤 영역의 리뷰가 요청되었는지를 알 수 있도록 돕기 때문입니다.
예를들어, backend
와 frontend
의 관심사가 있는 Merge Request의 경우 다음과 같이 리뷰어를 언급할 수 있습니다:
@john_doe 님, ~backend를 리뷰 부탁드립니다.
또는 @jane_doe, 이 MR을 ~frontend 메인테이너 리뷰 부탁드릴 수 있을까요?
또한 workflow::ready for review
라벨을 사용할 수도 있습니다. 이는 여러분의 Merge Request이 리뷰를 받을 준비가 되었음을 의미하며 어떤 리뷰어든 픽할 수 있음을 나타냅니다. 이 라벨을 사용하는 것은 시간 제약이 없는 경우에만 권장되며, Merge Request이 특정 리뷰어에게 할당되었는지 확인하세요.
Merge Request이 첫 번째 리뷰어로부터 승인을 받으면 유지자(maintainer)에게 전달될 수 있습니다. 도메인 전문성을 가진 유지자를 선택하는 것이 기본입니다. 그렇지 않은 경우 Reviewer Roulette 권장 사항을 따르거나 ready for merge
라벨을 사용하세요.
가끔 유지자가 리뷰에 사용할 여유가 없을 수 있습니다. 그들은 사무실을 비우거나 용량초과 상태일 수 있습니다. 당신은 그들의 프로필에서 유지자의 가용성을 확인할 수 있고 그들의 추천에 따르지 못했다면 다른 사람을 선택할 수 있습니다.
Merge Request을 리뷰하는 것은 Merge Request의 작성자의 책임입니다. 긴 시간 동안 ready for review
상태로 남아 있는 경우, 특정 리뷰어로부터 리뷰를 요청하는 것이 권장됩니다.
리뷰에 참여하기를 자원하는 방법
용량이 있는 GitLab 엔지니어들은 정기적으로 리뷰할 Merge Request 디렉터리을 확인하고 원하는 Merge Request에 리뷰어로 추가할 수 있습니다.
Merge Request 검토하기
변경이 왜 필요한지 이해하세요(버그를 수정하는 것, 사용자 경험을 향상시키는 것, 기존 코드를 리팩토링하는 것). 그런 다음:
- 반복 횟수를 줄이기 위해 철저하게 리뷰하려고 노력하세요.
- 의견이 강한 아이디어와 그렇지 않은 아이디어를 전달하세요.
- 문제를 해결하면서 코드를 단순화하는 방법을 찾으세요.
- 대안적 구현을 제안하지만 작성자가 이미 고려했을 것으로 가정하세요. (“여기에 사용자 정의 유효성 검사기를 사용하면 어떻게 생각하세요?”)
- 작성자의 시각을 이해하려 노력하세요.
- 브랜치를 확인하고 변경 사항을 로컬로 테스트하세요. 매뉴얼 테스트를 얼마나 진행할지는 여러분이 결정할 수 있습니다. 여러분의 테스트는 자동화된 테스트를 추가할 수 있는 기회로 이어질 수 있습니다.
- 코드 일부를 이해하지 못한다면 말하세요. 이것이 혼란스러운 부분이라면 다른 사람들도 혼동스러워할 가능성이 큽니다.
- 작성자가 필요한 내용이 무엇인지 명확하지 않도록 하세요.
- 공통 코멘트 형식을 사용하여 여러분의 의도를 전달하세요.
- 비필수적인 제안의 경우, (non-blocking)으로 꾸며 작성자가 Merge Request에서 선택적으로 해결할 수 있음을 알 수 있도록 하세요.
- Chrome/Firefox 애드온을 사용하여 공통 코멘트 접두어를 적용할 수 있습니다.
- 열린 의존성이 없는지 확인하세요. 막혔는지 확인하려면 링크된 이슈를 확인하세요. 필요하다면 작성자와 명확히 해야 합니다. 하나 이상의 오픈 MR에 의해 막혀 있다면 MR 의존성을 설정하세요.
- 라인 코멘트의 라운드 이후, “제가 보기에 괜찮아 보입니다” 또는 “처리해야 할 사항이 조금 있습니다”와 같은 요약을 게시하는 것이 도움이 될 수 있습니다.
- 리뷰 후에 변경 사항이 필요한 경우 작성자에게 알리세요.
Merge Request(MR) Merge하기
Merge을 결정하기 전에:
- 마일스톤을 설정합니다.
- 올바른 MR 유형 라벨이 적용되었는지 확인합니다.
- danger bot, 코드 품질 및 다른 보고서에서의 경고 및 오류를 고려합니다. 위반 사례에 대한 강력한 이유가 제시되지 않는 한, Merge하기 전에 이러한 사항을 해결해야 합니다. MR이 실패한 작업으로 Merge되면 코멘트를 게시해야 합니다.
- MR이 품질 및 비품질 관련 변경 사항을 모두 포함하고 있다면, 품질 관련 변경 사항이 테스트 엔지니어에 의해 승인된 후에, 사용자 기반 변경 사항(백엔드, 프론트엔드 또는 데이터베이스)에 대한 관련 담당자에 의해 MR이 Merge되어야 합니다.
적어도 하나의 담당자는 MR을 Merge하기 전에 승인해야 합니다. MR 작성자 및 MR에 커밋을 추가한 사람은 MR을 승인할 수 없으며, MR을 승인하기 위해 MR에 기여하지 않은 담당자를 찾아야 합니다. 일반적으로 최종 필요한 승인자가 MR을 Merge해야 합니다.
최종 승인자가 MR을 Merge하지 않을 수 있는 시나리오:
- 승인자가 승인한 후에 자동 Merge을 설정하는 것을 잊어버립니다.
- 승인자가 스스로가 최종 승인자임을 깨닫지 못합니다.
- 승인자가 자동 Merge을 설정했지만 GitLab에서 해제됩니다.
이러한 시나리오 중 하나라도 발생하면, MR 작성자는 모든 필수 승인을 받았고 리포지터리에 Merge 권한이 있을 경우 자신의 MR을 Merge할 수 있습니다. 이는 GitLab의 실행에 대한 편견가치와 일치합니다.
이 정책은 GitLab의 변경 관리 컨트롤 중 CHG-04 컨트롤을 만족시키기 위해 시행됩니다.
gitlab-org/gitlab
에서 이 정책을 시행하기 위해 다음 설정을 활성화했습니다.
gitlab-org/gitlab
의 CODEOWNERS
파일에 코드 소유자를 업데이트하려면 코드 소유자 승인 핸드북 섹션에서 설명된 프로세스를 따릅니다.
로컬에서 리베이스를 하거나 건의를 적용하는 것과 같은 일부 작업은 기존 승인을 재설정할 수 있습니다. UI에서 리베이스를 하거나 /rebase
퀵 액션으로 리베이스하는 경우에는 승인이 제거되지 않습니다.
Merge할 준비가 되었을 때:
- Merge Request에 많은 커밋이 있는 경우, Squash and merge 기능을 사용하는 것을 고려해보세요. 코드를 Merge할 때 유지자는 작성자가 이미 이 옵션을 설정한 경우에만 squash 기능을 사용하거나, MR이 명확하게 난잡한 커밋 기록을 포함하고 있을 경우에만 사용해야 합니다. 그렇지 않으면 MR에 커밋이 몇 개만 있으면 그대로 둬야 작성자의 설정을 존중합니다.
- Merge Request의 파이프라인 탭으로 이동하여 파이프라인 실행을 선택합니다. 그런 다음 개요 탭에서 자동 Merge을 활성화합니다.
NOTE:
- 기본 브랜치가 망가진 경우, Merge Request을 Merge하지 마세요. 몇 가지 매우 구체적인 경우를 제외하고는 Merge하지 마십시오. 다른 경우에는 핸드북 지침을 따르십시오.
- 최신 파이프라인이 승인되기 전에 만들어졌다면 완전한 RSpec 스위트가 실행되었는지 확인하기 위해 새 파이프라인을 시작하십시오. 이러한 단계는 MR이 백엔드 변경 사항을 포함하지 않는 한 건너뜁시다.
- 가장 최근의 Merge된 결과 파이프라인이 4시간 이내에 생성되었다면, 대상 브랜치에 대해 충분히 가까우므로 새로운 파이프라인을 시작하지 않고도 Merge할 수 있습니다.
- MR을 자동 Merge하려면, 이후에 발견될 수 있는 사항이 있으면 책임져야 합니다.
- Squash and merge를 설정한 Merge Request의 경우, 난잡한 커밋의 기본 커밋 메시지는 Merge Request 제목에서 가져옵니다. Merge하기 전에 더 유익한 커밋 메시지가 있는 커밋을 선택하는 것이 좋습니다.
Merge된 결과 파이프라인 덕분에 저자들은 이제 브랜치를 자주 리베이스할 필요가 없게 되었습니다(충돌이 있는 경우에만). 왜냐하면 Merge Results Pipeline은 이미 main
에서의 최신 변경 사항을 포함하기 때문입니다. 이로 인해 유지자들은 최종 리베이스를 요청할 필요가 없으므로 리뷰/Merge 주기가 빨라집니다: 대신에 MR 파이프라인을 시작하고 자동 Merge을 설정하기만 하면 됩니다. 이 단계는 실제로 Merge Trains 기능에 매우 가까워지게 해주는데, 이는 Merge Results가 파이프라인 생성 시점의 최신 main
에 대해 테스트되기 때문입니다.
커뮤니티 기여
넓은 커뮤니티 기여자가 추가한 Merge Request을 검토할 때:
- Ruby 젬 및 Node 패키지와 같은 새로운 의존성 및 의존성 업데이트에 특별히 주의합니다.
Gemfile.lock
또는yarn.lock
과 같은 파일의 변경은 간단해 보일 수 있지만, 악의적인 패키지를 가져올 수 있습니다. - 문서 Merge Request의 링크 및 이미지를 검토하세요.
- 의심스러울 때는
@gitlab-com/gl-security/appsec
에서 누군가에게 직접 Merge Request 파이프라인을 시작하기 전에 Merge Request을 검토하도록 요청하세요. - Merge Request이 현재 마일스톤에 포함될 가능성이 높은 경우에만 마일스톤을 설정합니다. 이렇게 함으로써 Merge될 때 혼란을 피하고 아직 준비되지 않은 상태에서 마일스톤을 자주 이동하는 일을 피할 수 있습니다.
MR 소스 브랜치가 대상 브랜치보다 1,000개 이상의 커밋을 떨어졌을 경우:
- 작성자에게 리베이스하거나 “목표 브랜치로 Merge할 수 있는 멤버에서 커밋 허용”이 활성화된 경우 직접 리베이스하는 것을 고려하거나 실행에 대한 편견을 가지고 직접 리베이스할 수 있습니다.
- 최근 변경 내용의 맥락에서 MR을 검토하면 숨겨진 런타임 충돌을 방지하고 일관성을 유지할 수 있습니다. 변경의 성격에 따라, MR이 1,000개 미만의 커밋만 떨어져 있다면 리베이스하는 것이 좋을 수 있습니다.
- 강제 푸시는 기여자를 당황케 할 수 있으므로, 리베이스를 수행했음을 알리거나, 기여자가 활동적으로 MR에 작업 중일 때 확인하세요.
- 리베이스는 일반적으로 GitLab 내에서
/rebase
빠른 조작을 사용하여 수행할 수 있습니다.
커뮤니티 Merge Request을 인수하는 경우
MR이 추가 변경이 필요하지만 작성자가 장기간 응답하지 않거나 MR을 완료할 수 없는 경우, GitLab은 이슈 및 Merge Request에 대한 종료 정책에 따라 인수할 수 있습니다. 일반적으로 MR 코치인 GitLab 엔지니어는 다음과 같은 단계를 따릅니다:
- MR에 대한 코멘트를 추가하여 MR이 Merge되도록 인수할 것임을 알립니다.
- MR에
~"coach will finish"
라벨을 추가합니다. - 본 브랜치에서 새로운 기능 브랜치를 생성합니다.
- 해당 브랜치를 새 기능 브랜치에 Merge합니다.
- 새 기능 브랜치를 본 브랜치에 Merge하기 위한 새로운 MR을 오픈합니다.
- 커뮤니티 MR을 본 MR에서 링크하고
~"Community contribution"
로 라벨을 지정합니다. - 필요한 최종 조정을 수행하고 기여자에게 변경 사항을 검토할 기회를 주고, 그들의 콘텐츠가 본 브랜치로 Merge된다는 사실을 알립니다.
- 콘텐츠가 모든 Merge Request 지침을 준수하는지 확인합니다.
- 일반적인 Merge Request처럼 정기적인 리뷰 프로세스를 따릅니다.
적절한 균형
코드 리뷰에서 가장 어려운 것 중 하나는 리뷰어가 작성자의 코드에 얼마나 개입해야 하는지에 대한 적절한 균형을 찾는 것입니다.
- 적절한 균형을 찾는 방법을 배우는 데는 시간이 걸립니다. 그래서 몇 시간 동안 Merge Request을 검토한 후 유지자가 되는 리뷰어가 있습니다.
- 버그를 찾는 것은 중요하지만 좋은 디자인에 대해 생각하는 것도 중요합니다. 추상화와 좋은 디자인을 구축하는 것은 복잡성을 숨기고 미래의 변경을 쉽게 만드는 것을 가능하게 합니다.
- 코드 스타일을 강화하고 개선하는 것은 주로 리뷰 코멘트보다는 자동화를 통해 해야 합니다.
- 작성자에게 디자인을 변경하도록 요청하는 것은 때로는 기여한 코드를 완전히 다시 작성해야 합니다. 이것을 하기 전에 다른 유지자나 리뷰어에게 물어보는 것이 일반적으로 좋은 아이디어지만, 중요하다고 생각될 때에는 용기를 내야 합니다.
- 반복의 이익을 위해 리뷰 제안 사항이 비차단 변경이거나 개인적인 취향(문서화되거나 합의된 요구 사항이 아님)이라면 작성자에게 되돌려주기 전에 Merge Request을 승인하는 것이 좋습니다. 이렇게 하면 작성자가 동의하는 경우 제안 사항을 구현할 수 있으며, 유지자에게 바로 전달할 수도 있습니다. 이렇게 하면 전체적인 Merge 시간을 단축할 수 있습니다.
- 일을 바르게 하는 것과 지금 당장 일을 바르게 하는 것에는 차이가 있습니다. 이상적인 상황에서는 전자를 해야 하지만 실제 세계에서는 후자가 필요할 때도 있습니다. 좋은 예는 가능한 빨리 발표해야 하는 보안 수정입니다. 긴급 수정이 필요한 Merge Request에서 작성자에게 주요 리팩터링을 요청하는 것은 피해야 합니다.
- 오늘 일을 잘하는 것이 내일 완벽하게 하는 것보다 보통 더 좋습니다. 지저분한 작업을 오늘 배포하는 것은 일반적으로 내일 제대로 하는 것보다 나쁩니다. 적절한 균형을 찾지 못할 때는 다른 사람들의 의견을 물어보세요.
GitLab 특정 고려 사항
GitLab은 많은 곳에서 사용됩니다. 많은 사용자가 Omnibus 패키지를 사용하지만, 일부 사용자는 도커 이미지를 사용하고, 일부는 소스에서 설치하며, 기타 설치 방법도 있습니다. GitLab.com 자체는 큰 엔터프라이즈 에디션 인스턴스입니다. 이에는 몇 가지 영향이 있습니다:
-
쿼리 변경은 GitLab.com 규모에서 성능이 저하되지 않도록 테스트해야 합니다:
- 지역적으로 대량의 데이터 생성이 도움이 될 수 있습니다.
- GitLab.com에서 쿼리 계획을 요청하는 것이 가장 신뢰할 수 있는 확인 방법입니다.
-
데이터베이스 마이그레이션은 다음과 같아야 합니다:
- 되돌릴 수 있는.
- GitLab.com 규모에서 성능이 우수해야 합니다 - 확신이 없다면 유지자에게 테스트하도록 요청하세요.
- 올바르게 분류되어 있어야 합니다:
- 새 코드가 인스턴스에서 실행되기 전에 실행되는 일반적인 마이그레이션.
- 포스트-배포 마이그레이션은 새로운 코드가 배포된 후, 인스턴스가 그렇게 설정되어 있을 때에 실행됩니다.
- 일괄 배치 백그라운드 마이그레이션은 Sidekiq에서 실행되며, 포스트-배포 마이그레이션 시간 제한을 초과하는 마이그레이션에 사용되어야 합니다.
-
Sidekiq 워커는 역호환성 없이 변경할 수 없습니다:
- Sidekiq 대기열은 배포 전에 비워지지 않으므로 이전 버전의 GitLab에서 대기 중인 워커가 있을 수 있습니다.
- 메서드 시그니처를 변경해야 하는 경우, 두 릴리스에 걸쳐 변경하려고 하고 첫 번째 릴리스에서 이전 및 새 인수를 모두 허용하세요.
- 마찬가지로 워커를 제거해야 하는 경우, 다음 릴리스에서 예약되지 않게 한 후 다음 릴리스에서 그 워커를 제거하세요. 이렇게 하면 기존 작업을 실행할 수 있습니다.
- 잊지 마세요. 모든 인스턴스가 모든 중간 버전으로 업그레이드되지는 않습니다(어떤 사람들은 X.1.0에서 X.10.0으로 넘어가거나 더 큰 업그레이드를 시도할 수도 있음!). 따라서 구 형식을 저렴하게 허용하는 것이 좋습니다.
- 캐시된 값은 릴리스 간에 지속될 수 있습니다. 캐시 값이 반환하는 유형을 변경하는 경우(예: 문자열 또는 nil에서 배열으로 변경), 동시에 캐시 키를 변경하세요.
- 설정은 최후의 수단으로 추가해야 합니다. GitLab Rails에 새로운 설정 추가를 참조하세요.
- 파일 시스템 액세스는 클라우드 네이티브 아키텍처에서 불가능합니다. 필요한 파일 리포지터리에 대해 객체 리포지터리를 지원하는지 확인하세요. 자세한 내용은 업로드 문서를 참조하세요.
고객 중요한 Merge Request
Merge Request은 비즈니스에 상당한 이점이 있는 경우, 고객 중요한 우선순위로 고려될 수 있습니다.
고객 중요 Merge Request의 특성:
- 개발의 시니어 디렉터 이상의 직원은 Merge Request이 고객에 중요하다고 승인해야 합니다. 다른 방법으로, 그들의 직속 부서장 두 명이 승인하면 이를 승인으로 간주합니다.
- DRI은 Merge Request에
customer-critical-merge-request
라벨을 적용합니다. - 고객 중요 Merge Request과 관련된 리뷰어와 유지보수 담당자가 이 결정이 내려지는 즉시 참여해야 합니다.
- 고객 중요 Merge Request을 처리하는 관련 담당자들의 작업에 우선순위를 두어 해당 작업에 집중할 수 있는 충분한 시간을 확보하는 것이 필요합니다.
- GitLab values 및 프로세스를 준수하여 고객 중요 Merge Request에 작업할 때, 가족과 친구를 우선하고 일을 뒤로 미루는 것, 완료 기준의 정의, 반복, 그리고 사용 가능할 때 릴리스하는 사항 등을 특히 유의해야 합니다.
- 고객 중요 Merge Request은 기술적인 결정에 우선순위를 두는 방법에 따라 보안을 감소시키지 않고, 데이터 손실 위험을 도입하지 않으며, 가용성을 감소시키지 않으며, 기존 기능을 손상시키지 않아야 합니다.
- 고객 중요 요청에 대해, 해당하는 담당자들이 동기적으로 조정하는 것이 권장되며, 이는 결합 시간을 줄일 수 있는지 고려해보는 것이며, 이는 효율성을 희생할 수 있다는 것을 감수해야 합니다.
- 고객 중요 Merge Request이 Merge된 후, 향후 고객 중요 Merge Request의 빈도를 줄이기 위해 회고가 완료되어야 합니다.
예시
코드 리뷰가 진행되는 방식은 새로운 기여자들에게 놀라울 수 있습니다. 여기에는 기대할 수 있는 것에 대한 방향을 제시해줄 일부 코드 리뷰의 예시가 있습니다.
“DiffNote
를 재사용하여 디자인 수정”:
이 MR에는 새 줄에 대한 첨부, 디자인 버전에 대한 논리를 포함하는 것부터, 특정 파일의 이전 버전이 없는 경우에는 어떻게 비교해야 하는지에 대한 이유까지 모든 것이 포함되어 있었습니다.
“다중 행 제안 지원”: MR 자체는 FE와 BE 간의 협력으로 이루어졌고, 작성자의 리뷰어에 대한 설명을 문서화하는 것이었습니다. 어떤 부분에 대한 첨부와 정보 문의가 있었고, 제일 끝 부분에는 보안 취약점이 있었습니다.
“프로젝트당 다중 리포지터리 허용”:
ZJ는 다른 프로젝트(워크호스)를 참조하여, 일관성을 위한 일부 개선을 제안했습니다. 그리고 James의 코멘트는 위임, &.
와 같은 것들의 코드 품질을 향상시키는 데 도움이 되었고, 코드를 보다 견고하게 만들었습니다.
“Merge Request을 위한 다중 담당자 지원”: 코드베이스 여러 부분을 건드리는 MR에 대한 협업의 좋은 예입니다. Nick는 흥미로운 엣지 케이스들을 지적했고, James Lopez 역시 가져온 기능에 대해 우려를 표명했습니다.
크레딧
주로 thoughtbot
코드 리뷰 가이드를 기반으로 합니다.