코드 리뷰 지침

이 안내서에는 코드 리뷰를 수행하고 코드 리뷰를 받기 위한 권장 사항과 모범 사례가 포함되어 있습니다.

GitLab CE 및 EE를 위한 모든 Merge Request은 GitLab 팀 구성원이거나 보다 넓은 커뮤니티 구성원이 작성한 것이든 상관없이, 코드가 효과적이고 이해하기 쉽고 유지 관리하기 쉽고 안전하도록 보장하기 위해 코드 리뷰 프로세스를 거쳐야 합니다.

Merge Request 검토, 승인 및 Merge 받기

시작하기 전에:

코드 검토를 위해 코드가 있는 즉시 리뷰어에게 코드를 검토해 달라고 요청하세요. 이 리뷰어는 귀하의 그룹이나 팀에서 올 수도 있고 도메인 전문가일 수도 있습니다. 리뷰어는:

  • 선택한 솔루션 및 구현에 대해 두 번째 의견을 제시할 수 있습니다.
  • 버그, 논리 문제 또는 처리되지 않은 변칙적인 경우를 찾는 데 도움을 줄 수 있습니다.

Merge Request이 간단하고 쉽게 검토할 수 있는 경우, 리뷰어 단계를 건너뛰고 직접 유지 관리자에게 요청할 수 있습니다.

“간단하고 직관적인”이란 것은 모호한 영역입니다. 여기 몇 가지 간단하고 직관적인 변경 사항의 예시가 있습니다:

  • 오탈자 수정 또는 작은 문구 변경 (예시).
  • 데이터나 동작을 변경하지 않는 작은 리팩터링.
  • 1달 이상 기본 활성화된 피처 플래그에 대한 참조 제거.
  • 사용되지 않는 메서드나 클래스 제거.
  • 5줄 미만의 코드를 변경해야 하는 잘 이해된 논리적 변경.

그렇지 않으면, 유지 관리자가 해당하는 각 카테고리(예: 백엔드, 데이터베이스)에서 처음으로 리뷰해야 합니다. 이는 유지 관리자가 해당 도메인 지식을 가지고 있을 수도 있기 때문이며 작업 부하를 분산시키기도 합니다.

보안 스캔이나 코멘트에 대한 지원이 필요한 경우, Application Security Team(@gitlab-com/gl-security/appsec)을 포함하세요.

리뷰어는 측면 표시줄의 리뷰어 기능을 사용합니다. 리뷰어는 추가로 승인함으로써 승인을 추가할 수 있습니다.

Merge Request이 닿는 영역에 따라 하나 이상의 유지 관리자에 의해 승인되어야 합니다. 승인 버튼은 Merge Request 위젯에 있습니다.

Merge Request을 Merge 받기 위해서는 유지 관리자도 필요합니다. 여러 승인이 필요한 경우, Merge을 검토하고 승인하는 마지막 유지 관리자가 Merge합니다.

일부 도메인 영역(Verify와 같은)은 CODEOWNERS 규칙에 따라 도메인 전문가의 승인이 필요할 수 있습니다. CODEOWNERS 섹션은 독립적인 승인 규칙이므로 우리는 일부 보다 일반적인 승인 규칙(예: 백엔드)의 하위 집합일 수 있는 특정 규칙(예: Verify)을 가질 수 있습니다. 더 효율적인 프로세스를 위해 저자는 일반적인 승인 전에 도메인별 승인을 찾아야 합니다. 도메인별 승인자는 유지 관리자가 될 수도 있으며, 그렇다면 그들은 해당 도메인의 특수 사항과 보다 넓은 변경을 동시에 검토하고 양쪽 역할에 대해 한 번에 승인해야 합니다.

저자 책임에 대해 더 자세히 알아보세요.

도메인 전문가

도메인 전문가는 특정 기술, 제품 기능 또는 코드베이스 영역에 상당한 경험을 가진 팀 구성원입니다. 팀 구성원은 도메인 전문가로 자체 식별하고 그것을 팀 프로필에 추가할 것을 권장받습니다.

도메인 전문가로 자체 식별할 때, .yml 파일을 변경하는 Merge Request을 기존에 설정된 도메인 전문가 또는 해당하는 엔지니어링 매니저가 Merge되도록 지정하는 것이 좋습니다.

다음은 자동적으로 도메인 전문가로 간주하는 데 대한 몇 가지 가정입니다:

  • 지정된 스테이지/그룹(예: create: source code)에서 일하는 팀 구성원은 해당 응용 프로그램 영역의 도메인 전문가로 간주됩니다.
  • 특정 기능(예: 검색)에 대한 작업을 하는 팀 구성원은 해당 기능의 도메인 전문가로 간주됩니다.

코드 리뷰를 위해 팀 구성원에게 도메인 전문가로 있을 수 있는 투표 권한을 할당합니다. 디자이너 용 모니터링 평가의 경우, 기본적으로 추천된 평가자에 따라 사용자 경험 평가를 필요로 합니다. 디자이너의 용량 한계로, 제품 디자이너가 지원하지 않는 영역은 커뮤니티 기여가 아니면 사용자 경험 평가가 더 이상 필요하지 않습니다. 적절한 도메인 전문가가 없는 경우, 팀 구성원 중 임의로 MR을 검토할 팀 구성원을 선택하거나 리뷰어 룰렛 권장을 따르십시오(위의 사용자 경험 평가에 대한 참조를 보려면 위로 올라가십시오).

도메인 전문가를 찾으려면:

  • Merge Request 승인 위젯에서 등록 가능 승인자 보기를 선택하세요. 이 위젯은 코드베이스 영역별로 권장 및 필수 승인을 보여줍니다. 이러한 규칙은 Code Owners에 정의되어 있습니다.
  • 머지 요청과 관련된 담당 팀원들의 디렉터리을 보세요.
  • 엔지니어링 프로젝트 페이지나 GitLab 팀 페이지에서 팀원의 도메인 전문성을 확인하세요. 도메인은 자체 식별되므로 당신의 판단을 사용하여 Merge Request 변경을 도메인에 매핑하세요.
  • Merge Request에 기여한 파일에 기여한 팀원들을 찾으세요. git log <file>를 실행하여 로그를 확인할 수 있습니다.
  • 파일을 검토한 팀원을 찾으세요. git log <file>를 사용하여 관련 Merge Request을 찾을 수 있습니다.
    1. git log <file>을 사용하여 커밋 SHA를 가져옵니다.
    2. https://gitlab.com/gitlab-org/gitlab/-/commit/<SHA>로 이동합니다.
    3. 커밋에 표시된 관련 Merge Request을 선택합니다.

리뷰어 룰렛

note
리뷰어 룰렛은 GitLab.com에서 사용하기 위한 내부 도구이며 고객 설치에는 사용할 수 없습니다.
note
%16.11까지, GitLab은 실험을 통해 배고픔 및 바쁨 표시를 제거하기 위해 실행됩니다.

Danger bot은 당신의 Merge Request이 영향을 미치는 코드베이스의 각 영역마다 리뷰어와 유지 관리자를 임의로 선택합니다. 개발자 리뷰어에게 권장사항을 제시하고 누군가 다른 사람이 더 적합하다고 판단되면 이를 재정의해야 합니다.

승인 지침은 도메인 전문가를 선택하는 데 도움이 될 수 있습니다.

우리는 제품 디자이너를 포함하는 팀의 MR에 대해서만 UX 리뷰를 수행합니다. 이러한 팀의 사용자 지향적인 변경사항은 기본적으로 UX 리뷰가 필요합니다. 심지어 피처 플래그에 숨겨져 있더라도 유저 중심적인 변경사항은 UX 리뷰가 필요합니다. 기본적으로 제안된 UX 리뷰어를 참조하세요.

그 동작은 다음과 같습니다.

  • Slack 또는 GitLab 상태가 다음을 포함하는 사람을 선택하지 않습니다:
    • OOO, PTO, 부모 휴가, 친구와 가족, 또는 컨퍼런스 문자열을 포함합니다.
    • GitLab 사용자 바쁨 표시가 True로 설정되어 있습니다.
    • Emoji가 다음 범주 중 하나입니다:
      • 휴가 중 - 🌴 :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은 보통 백포트이며 유지 관리자나 리뷰어가 그를 검토하는 데 많은 시간이 필요하지 않습니다.

  • 리뷰에 대한 “배고픔”: Slack 또는 GitLab 상태 이모지가 🔵 :large_blue_circle:인 팀 구성원이 더 많이 선택될 가능성이 높습니다. 이는 리뷰어와 연습 유지 관리자에게 모두 적용됩니다.
    • 🔵 :large_blue_circle:를 가진 리뷰어는 다른 리뷰어보다 2배 더 많이 선택됩니다.
    • 🔵 :large_blue_circle:를 가진 연습 유지 관리자는 다른 리뷰어보다 3배 더 많이 선택됩니다.
  • GitLab 상태 이모지가 🔶 :large_orange_diamond: 또는 🔸 :small_orange_diamond:인 사람은 절반으로 선택될 가능성이 떨어집니다.

  • 동일한 브랜치 이름에 대해 항상 동일한 리뷰어와 유지 관리자를 선택합니다(1번 항목과 같이 OOO 상태가 변경되는 경우 제외). 앞뒤로 ce-와 -ee를 제거한 뒤 안정적으로 백포트 브랜치를 유지하기 위해 이에 해당하는 사람을 제거합니다.

승인 지침

유지자의 책임에 관한 섹션에서 설명된 대로, 도메인 전문가에 의해 권장사항으로 여겨지는 내용은 유지자에 의해 승인 및 Merge을 받아야 합니다. 첫 번째 리뷰어의 선택적인 승인은 여기에 포함되지 않습니다. 그러나 Merge Request은 유지자에게 전달되기 전에 리뷰어에 의해 검토되어야 합니다. 이에 대한 설명은 개요 섹션을 참고하세요.

Merge Request에 포함된 내용 승인이 필요한 담당자
~backend 변경 1 백엔드 유지자.
~database 마이그레이션 또는 비용이 많이 드는 쿼리 변경 2 데이터베이스 유지자. 자세한 내용은 데이터베이스 리뷰 안내문을 참조하세요.
~workhorse 변경 워크호스 유지자.
~frontend 변경 1 프론트엔드 유지자.
~UX 사용자 인터페이스 변경 3 제품 디자이너. 자세한 내용은 디자인 및 사용자 인터페이스 안내문을 참조하세요.
새로운 JavaScript 라이브러리 추가 1 - 프론트엔드 foundations 구성원, 라이브러리가 번들 크기를 크게 늘리는 경우.
- 법무팀 구성원, 새 라이브러리의 라이선스가 GitLab에서 사용을 승인받지 않은 경우.

라이선스 호환성에 대한 자세한 정보는 GitLab 라이센싱 및 호환성 문서에서 확인할 수 있습니다.
새로운 의존성 또는 파일 시스템 변경 - Distribution 팀 구성원. 자세한 내용은 Distribution 팀과 함께 작업하는 방법을 참조하세요.
- RubyGems의 경우, AppSec 리뷰를 요청하세요.
~documentation 또는 ~UI 텍스트 변경 적절한 DevOps 단계 그룹의 과제를 기반으로 한 기술 문서 작가.
개발 가이드라인 변경 리뷰 프로세스를 따르고 해당하는 승인을 얻으세요.
End-to-end 및 비엔드-to-엔드 변경 4 품질 유지자.
오직 End-to-end 변경 4 또는 MR 작성자가 품질 유지자인 경우 품질 유지자.
새로운 또는 업데이트된 애플리케이션 한도 제품 관리자.
분석 계측 (텔레메트리 또는 분석) 변경 분석 계측 엔지니어.
기능 스펙 추가 또는 변경 품질 유지자 또는 품질 리뷰어.
GitLab에 새로운 서비스 추가 (Puma, Sidekiq, Gitaly의 예가 있음) 제품 관리자. 자세한 내용은 GitLab에 서비스 컴포넌트를 추가하는 프로세스를 참조하세요.
인증 관련 변경 관리:인증. 상세한 내용은 그룹 페이지의 코드 리뷰 섹션을 확인하세요. 해당 파일이 리뷰가 필요한 팀의 지정된 CODEOWNERS 파일의 Authentication 섹션에 나열된 파일 패턴과 해당 MR의 승인자 섹션을 확인합니다.
사용자 정의 역할 또는 정책 관련 변경 관리:인가 엔지니어.
  1. JavaScript 스펙 외의 스펙은 ~backend 코드로 간주됩니다. Haml 마크업은 ~frontend 코드로 간주됩니다. 그러나 의심스러운 경우, 프론트엔드 및 백엔드 리뷰를 요청하세요.
  2. Merge Request이 비용이 많이 드는 쿼리를 도입할 가능성이 있는 경우, 데이터베이스 유지자의 조언을 구하는 것을 권장합니다. 그들이 자문을 제공할 수 있도록 해당 SQL 쿼리가 있는 코드 라인에 코멘트하는 것이 가장 효율적입니다.
  3. 사용자 인터페이스 변경에는 시각적 변경 (얼마나 작더라도)과 화면 판독기가 콘텐츠를 발표하는 방식에 영향을 주는 렌더링된 DOM의 변경이 포함됩니다. 전문적인 제품 디자이너를 갖추지 않은 그룹들은 제품 디자이너가 커뮤니티 기여가 아닌 경우 기능 변경을 승인할 필요가 없습니다.
  4. End-to-end 변경은 qa 디렉터리의 모든 파일을 포함합니다.

CODEOWNERS 승인

일부 Merge Request은 특정 그룹의 의무적인 승인을 요구합니다. 정의는 .gitlab/CODEOWNERS에서 확인하세요.

.gitlab/CODEOWNERS의 의무적인 섹션은:

  • 규정 준수
  • 가용성
  • 보안

에 필요한 경우에만 제한적으로 사용해야 합니다.

의무적인 섹션을 추가할 때에는 새로운 의무 섹션이 Merge Request 비율에 미치는 영향을 추적해야 합니다. 좋은 예제는 Verify issue를 참고하세요.

그 외의 모든 경우에는 우리는 엄격함보다 책임성을 선호합니다.

또한, 현재의 모놀리스 구조는 Merge Request이 보이지 않는 부분에 영향을 줄 가능성이 높습니다. 여러 의무적인 승인은 이러한 Merge Request에 대한 작성자의 승인을 요구하므로 효율적이지 않습니다.

이를 개선하기 위한 노력은 다음에서 확인할 수 있습니다:

승인 체크리스트

이 체크리스트는 Merge Request(MR)의 작성자, 리뷰어 및 유지보수자가 변경 사항을 품질, 성능, 신뢰성, 보안, 가시성 및 유지 관리성에 대한 고위험 분석을 확인하도록 장려합니다.

체크리스트 사용은 소프트웨어 엔지니어링에서 품질을 향상시킵니다. 이 체크리스트는 GitLab 코드베이스 기여자의 기술을 지원하고 강화하는 간편한 도구입니다.

품질

더 많은 품질 가이드라인은 테스트 엔지니어링 프로세스를 참조하세요.

  1. 당신은 코드 리뷰 가이드라인에 따라 이 MR을 자체 검토했습니다.
  2. 이 변경이 영향을 미치는 코드에 대해 자동화된 테스트 (테스트 가이드)가 사용자에게 매우 중요한 기능을 확인하는지 믿습니다 (모든 테스트 수준을 고려).
  3. 기존의 자동화된 테스트가 위의 기능을 커버하지 않는 경우, 해당 추가 테스트를 추가하거나 자동화 테스트 간격을 설명하는 이슈를 추가하고 이 MR에 연결했다고 가정합니다.
  4. GitLab.com 호스트된 고객 및 Self-Managed형 고객에 대한 이 변경 사항의 기술적 측면의 영향을 고려했습니다.
  5. 이 변경 사항이 시스템의 프론트엔드, 백엔드 및 데이터베이스 부분에 미치는 영향을 적절하게 고려하고 ~ux, ~frontend, ~backend, ~database 레이블을 적용했습니다.
  6. 이 MR을 지원되는 모든 브라우저에서 테스트했거나, 이러한 테스트가 필요하지 않음을 결정했습니다.
  7. 이 변경 사항이 업데이트 간에 하위 호환성을 가지고 있는지 확인했거나, 이 적용이 필요하지 않다고 결정했습니다.
  8. EE 콘텐츠를 적절하게 분리했거나 이 MR이 FOSS 전용입니다.
  9. 이 MR이 EE 및 FOSS에 서로 다른 방식으로 영향을 줄 수 있다고 생각한다면, FOSS 컨텍스트에서 CI 파이프라인을 실행하는 것을 고려했습니다.
  10. 기존 데이터가 놀랍도록 다양할 수 있다는 것을 고려했습니다. 예를 들어 새로운 모델 유효성 검사는 기존 레코드를 손상시킬 수 있습니다. 기존 데이터가 유효성 검사를 통과할 것이 확실하지 않다면, 기존 데이터에 대한 유효성 검사를 필수가 아니라 선택 사항으로 만들 것을 고려했습니다.
  11. 테스트가 경고와 함께 통과하고 실패한 작업이 이 MR에 의해 변경된 파일 디렉터리에서 Flaky test '<path/to/test>' was found in the list of files changed by this MR. 텍스트를 포함하는 경우, 이 테스트를 수정하거나 왜 이 테스트를 무시해도 되는지 설명하는 증거를 제공한 것으로 가정합니다.
성능, 신뢰성 및 가용성
  1. 이 MR이 성능에 해를 끼치지 않는다고 확신합니다. 또는 리뷰어에 대한 성능 영향을 평가하는 데 도움을 청했습니다 (Merge Request 성능 가이드라인).
  2. MR 설명에 데이터베이스 리뷰어를 위한 정보를 추가했거나 이것이 불필요하다고 결정했습니다.
    • 이 MR에 데이터베이스 관련 변경 사항이 있는지 확인했습니까? (database_review.md)
  3. 이 변경의 가용성 및 신뢰성 위험을 고려했습니다.
  4. 미래 예측 성장을 기반으로 한 확장 가능성 위험을 고려했습니다.
  5. 이 변경 사항이 평균 고객보다 훨씬 더 많은 데이터를 가질 수 있는 대규모 고객에게 미치는 성능, 신뢰성 및 가용성 영향을 고려했습니다.
  6. 이 변경 사항이 GitLab을 최소 시스템에서 실행하는 고객에 대한 성능, 신뢰성 및 가용성 영향을 고려했습니다.
가시성 계기
  1. 이 변경에 대한 디버깅 및 선제적인 성능 향상을 용이하게 하는 충분한 계기를 포함했습니다. 예시를 참조하여 피처 플래그, 로깅 및 계기 추가.
문서
  1. 변경 일지 트레일러를 포함했거나 이것이 필요하지 않다고 결정했습니다.
    • 이 MR이 변경 일지가 필요합니까? (changelog.md#what-warrants-a-changelog-entry)
  2. 문서를 추가/업데이트하거나 이 MR에 대한 문서 변경이 불필요하다고 결정했습니다.
    • 제품 변경에 대한 문서가 필요합니까? (https://handbook.gitlab.com/handbook/product/ux/technical-writing/workflow/#documentation-for-a-product-change)
보안
  1. 만약 이 MR이 자격 증명이나 토큰, 권한, 인증 방법에 대한 처리 또는 저장을 변경한다면, 또는 다른 사항을 설명하는 보안 검토 가이드라인에 기술된 경우, ~security 레이블을 추가했으며, @gitlab-com/gl-security/appsec를 언급했습니다.
  2. 내부 애플리케이션 보안 검토에 관한 문서를 검토했으며, 이 변경에 대한 보안 검토가 필요하다고 판단되면 언제, 어떻게 보안 검토를 요청하는지에 대한 문서에 요청하여 보안 검토를 요청했습니다.
  3. 만약 보안 스캔 결과가 MR을 차단하는 경우 (Merge Request 승인 정책으로 인해):
    • 실제 양성 결과에 대해서는 MR이 Merge되기 전에 수정해야 합니다. 이것은 해당 Merge Request 승인 정책에 의해 AppSec 승인이 필요 없어질 것입니다.
    • 객체적 양성 결과에 대해서는 위험 수용을 논의해야 하며, AppSec에 ping @gitlab-com/gl-security/appsec 해야 할 사항입니다.
배포
  1. 이 변경 사항이 고위험일 수 있으므로 피처 플래그를 사용하는 것을 고려했습니다.
  2. 피처 플래그를 사용할 경우, 제품을 실제환경에서 테스트하기 전에 스테이징에서 변경 사항을 테스트할 계획이 있으며, 모든 고객에게 전파되기 전에 일부 고객에게 먼저 전파할 계획을 고려했습니다.
  3. 다음 사항 정의에 따라 기본 설정 또는 새 설정 변경에 대해 인프라 부서에게 알렸거나, 이것이 필요하지 않다고 결정했습니다.
준수
  1. 올바른 MR 유형 레이블이 적용되었는지 확인했습니다.

Merge Request 작성자의 책임

가장 좋은 해결책을 찾고 이를 구현하는 책임은 Merge Request 작성자에게 있습니다. 작성자 또는 직접 책임 있는 개인(DRI)은 코드 리뷰 수명주기 전체 동안 Merge Request에 할당된 상태로 유지합니다. 귀하는 자신을 assignee로 설정할 수 없다면 리뷰어에 요청하여 대신 수행하도록 요청하십시오.

승인 및 Merge을 위해 메인테이너의 검토를 요청하기 전에, 그들은 확신해야 합니다:

  • 그것은 그 문제를 실제로 해결했습니다.
  • 가장 적절한 방법으로 그것을 수행했습니다.
  • 모든 요구 사항을 충족했습니다.
  • 남아있는 버그, 논리적인 문제, 발생하지 않은 예외 상황 또는 알려진 취약점이 없습니다.

이러한 확신 수준에 도달하고 리뷰어와의 불필요한 반복을 피하기 위한 최선의 방법은 자신의 Merge Request을 스스로 검토하고 코드 리뷰 가이드라인을 따르는 것입니다. 이 자체 리뷰 중에, 결정이나 트레이드 오프가 이루어진 행에서 MR에 댓글을 포함하거나 문맥적 설명이 리뷰어가 코드를 더 쉽게 이해하는 데 도움이 될 수 있는 지점에 댓글을 포함하십시오.

자신의 솔루션에 대한 필요한 수준의 확신을 얻기 위해, 작성자는 타당한 경우 조사 및 구현 프로세스에 다른 사람들을 참여시킬 것으로 기대됩니다.

자신의 해결책에 대한 신뢰도를 확보하기 위해, 작성자는 다른 솔루션을 논의하거나 검토받기 위해 도메인 전문가에게 연락하거나 제품 관리자 및 UX 디자이너에게 혼동을 해소하거나 최종 결과가 그들이 상상했던 것과 일치하는지 확인할 수 있도록 하여 개발자, 데이터베이스 전문가, 기타 개발자에게 솔루션에 대한 깊은 검토를 받을 것을 권장합니다.

하나의 기능을 제공하기 위해 많은 Merge Request이 필요할 것이라고 확신하는 경우 (예: 증거 개념을 만들었고 기능이 10개 이상의 Merge Request으로 구성될 것으로 명확한 경우), 해당 기능을 이해하는 데 필요한 이해를 가진 리뷰어 및 메인테이너를 식별하고 직접한테 정보를 공유합니다. 그런 다음 모든 Merge Request을 이러한 리뷰어에게 직접 적용하십시오. 이러한 리뷰어를 찾는 데 최선의 DRI는 EM 또는 스탭 엔지니어입니다. 동일한 컨텍스트로 여러 Merge Request에 대한 안정적인 리뷰어 대응을 갖는 것은 효율성을 향상시킵니다.

만약 당신의 Merge Request이 하나 이상의 도메인에 영향을 미쳤다고 판단된다면 (예: Dynamic Analysis 및 GraphQL), 각 도메인의 전문가로부터 리뷰를 요청하십시오.

작성자가 Merge Request이 도메인 전문가의 의견을 필요로 하는지 확신할 수 없다면, 그것은 그것이 필요할 가능성이 있다는 것을 나타냅니다. 그렇지 않으면, 그들이 해결책에 필요한 수준의 확신을 가지고 있지 않을 가능성이 높습니다.

검토 전에, 작성자는 댓글을 제출하여 MR 차이에서 리뷰어에게 중요한 사항 및 추가 설명이나 주의사항을 요청하는 것이 요청합니다. 댓글에 포함될 수 있는 콘텐츠 예시는 아래와 같습니다:

  • 린팅 규칙 추가 (RuboCop, JS 등).
  • 라이브러리 추가 (Ruby gem, JS lib 등).
  • 명확하지 않은 경우, 상위 클래스 또는 메서드에 대한 링크.
  • 변경을 보완하는 교차검증 수행.
  • 잠재적으로 안전하지 않은 코드.

검토어를 할당할 때, 다음과 같은 작업이 유용할 수 있습니다:

  • MR에 댓글을 추가하여 해당 리뷰어로부터 어떤 유형의 리뷰를 기대하는지 명시하세요.
    • 예: 만

리뷰어의 책임

리뷰어는 선택한 솔루션의 세부 정보를 검토하는 책임이 있습니다.

Merge Request 검토를 철저히 하세요.

Merge Request이 기여 수락 기준을 모두 충족하는지 확인하세요.

일부 Merge Request은 특정 사역분야 전문가의 도움이 필요할 수 있습니다. 리뷰어는 해당 분야 전문가가 아닌 경우 다음 중 하나를 수행할 수 있습니다.

  • Merge Request을 검토하고 다른 전문가에게 다시 검토를 요청합니다. 이 전문가는 다른 리뷰어이거나 유지 보수자일 수 있습니다.
  • 더 적합한 다른 리뷰어에게 리뷰를 제공합니다.
  • 사역분야 전문가가 없는 경우, 최선을 다해 검토합니다.

Merge Request이 다음 중 하나인 경우 제안합니다. - 너무 큽니다. - 여러 문제를 해결합니다. - 여러 기능을 구현합니다. - 추가 리스크를 초래하는 높은 복잡성이 있습니다.

저자가 Merge Request을 작은 Merge Request으로 나누도록 안내하도록 하세요. 나눈 Merge Request에 대해 현재 유지 보수자 및 리뷰어가 검토하도록 요청하거나 새로운 유지 보수자 및 리뷰어 그룹을 요청할 수 있습니다.

모든 요구 사항을 충족시키는 것이 확실해지면 다음을 수행해야 합니다.

  • 승인을 선택합니다.
  • @를 사용하여 저자를 언급하여 to-do 알림을 생성하고, Merge Request이 검토되고 승인되었음을 알립니다.
  • 사역 분야 전문가의 검토를 요청합니다. 사역 분야 전문가의 경우 기본적으로 요청합니다. 그러나 확인되지 않는 경우나 Merge Request이 사역 분야 전문가의 검토가 필요하지 않다고 판단되면 리뷰어 룰렛 제안에 따라 진행하세요.
  • 스스로를 리뷰어에서 제거합니다.

유지 보수자의 책임

유지 보수자는 GitLab 코드베이스의 전반적인 건강 상태, 품질 및 일관성에 책임을 지고 있습니다. 따라서 그들의 리뷰는 주로 전반적인 아키텍처, 코드 구성, 관심사 분리, 테스트, DRYness, 일관성 및 가독성과 같은 것에 중점을 둡니다.

유지 보수자의 역할은 특정 도메인에 대한 지식이 아니라 GitLab 코드베이스의 전반에 걸친 것에만 의존하므로, 그들은 모든 팀 및 모든 제품 영역의 Merge Request을 검토, 승인 및 Merge할 수 있습니다.

유지 보수자는 Merge Request의 수락 기준이 합리적으로 충족되었는지 보증하는 책임을 지게 됩니다. 일반적으로 품질은 모두의 책임이지만, MR의 유지 보수자는 해당 일반적인 품질 표준을 충족하는 MR임을 보증하는 책임을 집니다.

이는 훗날 나올 문제의 기술적 부채 생성을 피합니다.

유지 보수자는 MR이 충분히 중요하다고 느끼거나 사역 분야 전문가가 필요한 경우, 다른 리뷰어 또는 유지 보수자에게 검토를 요청할 권한이 있습니다. 다음은 리뷰 중에 유지 보수자가 이 목적으로 주도적으로 이루어진 몇 가지 예시입니다.

유지 보수자는 Merge 전에 선택한 솔루션의 세부 정보를 검토하기 위해 최선을 다하지만, 그들이 반드시 사역 분야 전문가가 아닌 경우 합리적이 주를 쏟기 전까지 이를 실시하기가 어려울 수 있습니다. 이 경우에는 그들은 저자 및 이전 검토자의 판단을 우선시하여 본인의 주요 책임에 집중합니다.

Merge Request을 검토한 리뷰어인 동시에 유지 보수자인 개발자의 경우, 최종적으로 승인하고 Merge할 유지 보수자로 선택되지 않도록 권장됩니다.

Merge Request이 필요한 승인자에 의해 승인되었는지 유지 보수자는 Merge 전에 확인해야 합니다. 여전히 다른 사람들로부터 추가 승인을 기다리는 경우, 리뷰어 역할을 제거하고 해당 이유에 대해 작성된 코멘트로 저자를 @ 언급합니다. 코드를 Merge하는 경우에는 리뷰어로 유지합니다.

특정 Merge Request은 안정적인 브랜치를 대상으로 할 수 있습니다. 이러한 요청을 처리하는 방법에 대한 개요는 패치 릴리스 런북을 참조하세요.

Merge 후에 유지 보수자는 Merge Request에 나열된 리뷰어로 유지해야 합니다.

리뷰어 기능의 독식

2021년 3월 18일에, 효율적이고 일관된 리뷰어 기능을 독식하기 위한 업데이트된 프로세스가 시행되었습니다.

다음은 변경 사항의 요약입니다. 이 내용은 위의 섹션에도 반영되어 있습니다.

  • Merge Request 저자 및 운영 책임자(DRIs)는 담당자(Assignees)로 유지합니다.
  • 저자는 리뷰어로 사용자를 지정하여 리뷰를 요청합니다.
  • 리뷰어는 리뷰가 끝나면 담당을 취소합니다.
  • (MR)를 Merge하는 마지막 승인자는 리뷰어로 유지합니다.

Best practices

모두가

  • 친절해야 합니다.
  • 많은 프로그래밍 결정이 의견입니다. 교섭하고 선호하는 것과 빨리 결론에 도달하세요.
  • 질문을 하세요. 요구하지 말고요. (“:user_id라는 이름을 사용하는 것에 어떻게 생각하세요?”)
  • 설명을 요청하세요. (“이해하지 못했습니다. 설명할 수 있나요?”)
  • 코드를 선택적으로 소유하지 마세요. (“내 것”, “내 것이 아님”, “당신 것”)
  • 개인적인 특성을 가리킬 수 있는 용어를 사용하지 마세요. (“멍청한”, “바보 같은”). 모두가 지적이고 선량하다고 가정하세요.
  • 명확해야 합니다. 온라인에서 의도를 이해하는 것은 언제나 쉽지 않습니다.
  • 겸손해야 합니다. (“자신이 확신을 못합니다 - 찾아봅시다.”)
  • 과장하지 마세요. (“항상”, “절대로”, “끝없이”, “아무것도”)
  • 무례한 행동으로 오해받을 수 있기 때문에 빈정거림의 사용에 주의하세요. 우리가 하는 모든 것은 공개적인 것입니다. 장기간 동료와의 상호 조롱처럼 보이는 것이 시작 프로젝트에 새로 온 사람에게는 냉혹하고 환영하지 않는 것으로 보일 수 있습니다.
  • 개별적인 대화나 영상 통화를 고려하세요. “이해 못 했습니다”나 “대체로 다른 해결책:”이라는 말이 너무 많이 나오는 경우. 개별적인 대화를 요약하는 후속 코멘트를 작성합니다.
  • 특정 사람에게 질문을 할 경우, 코멘트를항상 그 사람을 언급하여 시작하세요. 이렇게 하면 그들의 알림 수준이 “언급”으로 설정된 경우 메시지를 볼 수 있고 다른 사람에게 응답할 필요가 없음을 알 수 있습니다.

Merge Request 검토 방법

코드 검토는 여러 번 반복되는 프로세스일 수 있으며, 리뷰어는 처음에는 볼 수 없었던 내용을 후에 찾을 수도 있습니다.

  • 코드의 첫 번째 리뷰어는 _당신_입니다. 새로운 브랜치를 푸시하기 전에 전체 diff를 읽어보세요. 이해되나요? 변경 내용의 전반적인 목적과 관련이 없는 내용이 포함되지는 않았나요? 디버깅 코드를 제거해 두지 않았나요?
  • Merge Request 지침에서 제시된 대로 상세한 설명을 작성하세요. 일부 리뷰어는 제품 기능이나 코드베이스 영역에 익숙하지 않을 수 있습니다. 철저한 설명은 모든 리뷰어가 요청을 이해하고 효과적으로 테스트할 수 있도록 돕습니다.
  • 변경 사항이 다른 사항의 Merge이 먼저 되어야 한다는 것을 알고 있다면 설명에 기록하고 Merge Request 의존성을 설정하세요.
  • 리뷰어의 제안에 감사드세요. (“좋은 의견. 해당 사항을 변경하겠습니다.”)
  • 개인적으로 받아들이지 마세요. 리뷰는 코드에 대한 것입니다.
  • 코드가 왜 있는지 설명하세요. (“이것은 그 이유로 그런 모양입니다. 이 클래스/파일/메서드/변수를 변경하면 더 명확할까요?”)
  • 관련이 없는 변경 및 리팩터링을 향후 Merge Request/이슈로 추출하세요.
  • 리뷰어의 관점을 이해하려고 노력하세요.
  • 모든 코멘트에 응답하도록 노력하세요.
  • Merge Request 작성자는 완전히 해결한 스레드만 해결합니다. 여전히 오픈된 답글, 오픈된 스레드, 제안, 질문 또는 기타 모든 것이 리뷰어에 의해 해결되도록 남겨야 합니다.
  • 모든 피드백이 해당 권고된 변경사항이 MR이 Merge되기 전에 반드시 반영되어야 한다고 가정해서는 안 됩니다. 이것은 MR 작성자와 리뷰어의 판단에 따라 결정되거나 현재 해당가 필요하거나 해당 MR의 팔로업 이슈를 작성하여 향후 피드백에 대응해야 할 수 있습니다.
  • 이전 피드백을 기반으로 한 커밋을 분리된 커밋으로 브랜치에 푸시하세요. 브랜치가 Merge할 준비가 될 때까지 squash하지 마세요. 리뷰어는 이전 피드백에 기초한 각 업데이트를 읽을 수 있어야 합니다.
  • 다시 검토를 준비했을 때 리뷰어로부터 새로운 리뷰를 요청하세요. 리뷰를 요청할 수 있는 기능이 없는 경우 리뷰어를 @ 언급하세요.

리뷰 요청

Merge Request을 검토할 준비가 되면, 승인 가이드라인에 따라 리뷰어를 선택하여 초기 검토를 요청해야 합니다.

Merge Request에 여러 영역에 대한 검토가 필요한 경우, 리뷰어가 어떤 영역을 검토해야 하는지 및 어떤 단계에서(첫 번째 또는 두 번째) 검토해야 하는지를 명시하는 것이 좋습니다. 이는 여러 영역에 대한 자격을 갖춘 팀 멤버가 어떤 영역을 검토해야 하는지를 파악하는 데 도움이 됩니다. 예를 들어, Merge Request에 backend 및 frontend 관련 내용이 있는 경우 다음과 같이 리뷰어를 언급할 수 있습니다: @john_doe님, ~backend를 검토해주시겠습니까? 또는 @jane_doe님, 이 MR에 ~frontend 관리자 리뷰를 해주시겠어요?

또한 workflow::ready for review 레이블을 사용할 수도 있습니다. 이는 Merge Request을 검토할 준비가 되었음을 의미하며, 어떤 리뷰어든 가져갈 수 있습니다. 이 레이블은 시간이 촉박하지 않은 경우에만 사용하는 것이 좋으며, Merge Request이 리뷰어에게 할당되었는지 확인하세요.

Merge Request이 첫 번째 리뷰어로부터 승인을 받으면 유지보수자(maintainer)에게 전달될 수 있습니다. 도메인 전문 지식을 갖춘 유지보수자를 선택하는 것이 기본이며, 그렇지 않은 경우에는 리뷰어 룰렛 권장사항을 따르거나 ready for merge 레이블을 사용하세요.

때로는 유지보수자가 리뷰를 진행할 여유가 없을 수 있습니다. 외근 중이거나 용량이 가득 찼을 수 있습니다. 유지보수자의 가용성을 확인하고 플로텍 추천한 유지보수자가 가용하지 않다면 디렉터리에서 다른 사람을 선택하세요.

Merge Request 작성자가 검토를 받을 책임이 있습니다. ready for review 상태를 너무 오래 유지하는 경우, 특정 리뷰어에게 검토를 요청하는 것이 좋습니다.

리뷰 지원

여유가 있는 GitLab 엔지니어는 정기적으로 검토할 Merge Request 디렉터리을 확인하고 원하는 Merge Request에 리뷰어로 추가할 수 있습니다.

Merge Request 검토

변경 사항이 왜 필요한지(버그를 수정함, 사용자 경험을 향상시킴, 기존 코드를 리팩토링함) 이해하세요. 그런 다음:

  • 반복 횟수를 줄이기 위해 철저한 검토를 하십시오.
  • 강력하게 느끼는 아이디어와 그렇지 않은 아이디어를 공유하십시오.
  • 문제를 해결하는 동시에 코드를 간소화할 방법을 찾으세요.
  • 대체 구현을 제시하되, 작성자가 이미 고려했다고 가정하세요. (“여기서 사용자 정의 유효성 검사기를 사용하는 것은 어떠세요?”)
  • 작성자의 시각을 이해하려고 노력하세요.
  • 브랜치를 확인하고 변경 사항을 로컬에서 테스트하세요. 얼마나 많은 매뉴얼 테스트를 수행할지 결정할 수 있습니다. 테스트는 자동화된 테스트를 추가할 수 있는 기회로 이어질 수 있습니다.
  • 코드 일부를 이해하지 못한다면, 불확실하게 표현하세요. 다른 사람들도 혼란스러워할 가능성이 높습니다.
  • 작성자가 어떤 것이 필요한지 명확하게 이해하도록 하십시오.
    • 의도를 전달하기 위해 Conventional Comment 형식을 사용하는 것을 고려하십시오.
    • 필수가 아닌 제안에는 (non-blocking)을 붙여 작성자가 Merge Request 내에서 선택적으로 해결할 수 있음을 알 수 있게 하십시오. 유일한 제안이 선택적으로 해결할 수 있는 경우, 비동기 사이클을 줄이기 위해 MR을 다음 단계로 이동하십시오. 첫 번째 라운드 리뷰어인 경우, 유지보수자에게 전달하여 리뷰하게 하십시오. 마지막 승인 유지보수자인 경우, 비동작하는 제안으로부터 추적 사항을 생성하고 Merge하거나 자동 Merge을 설정하십시오. 그런 다음 작성자는 자동 Merge을 취소하거나 비동작하는 제안을 구현하여 MR을 제출하거나 제안을 구현하지 않기로 결정할 수 있습니다.
    • Chrome/Firefox add-on을 사용하여 Conventional Comment 접두어를 적용할 수 있습니다.
  • 열린 의존성이 없는지 확인하십시오. 블로킹 요소에 대한 링크된 이슈을 확인하십시오. 필요한 경우 작성자에게 명확히해야 합니다. 하나 이상의 열린 MR에 의해 차단되는 경우, MR 의존성을 설정하십시오.
  • 라인 노트를 한 번 했으면 “보기 좋아 보입니다” 또는 “해결해야 할 사항이 몇 가지 있습니다.”와 같은 요약 노트를 게시하는 것이 도움이 됩니다.
  • 검토에 따라 변경이 필요한 경우 작성자에게 알려주십시오.
caution
Merge Request이 forked된 경우, 커뮤니티 기여를 위한 추가 지침도 확인하세요.

Merge하기

Merge을 결정하기 전에:

  • 마일스톤을 설정하십시오.
  • 올바른 MR 유형 레이블이 적용되었는지 확인하십시오.
  • danger bot, 코드 품질 및 기타 보고서로부터의 경고와 오류를 고려하십시오. 위배 사례에 강력한 근거가 없는 한 Merge 전에 이러한 사항을 해결해야 합니다. 만일 어떤 작업이 실패한 상태로 Merge된 경우, 해당 사항에 대한 코멘트를 남겨야 합니다.
  • 품질과 비품질 관련 변경 사항이 모두 포함된 경우, 품질 관련 변경 사항이 테스트 엔지니어에 의해 승인된 후에 사용자에게 노출되는 변경 사항에 대해 관련 유지보수자(백엔드, 프론트엔드 또는 데이터베이스)에 의해 Merge되어야 합니다.

적어도 하나의 유지보수자가 MR을 승인해야 Merge할 수 있습니다. MR 작성자 및 MR에 커밋을 추가한 사람들은 MR을 승인할 권한이 없으며, 일반적으로 마지막 승인자가 MR을 Merge해야 합니다.

최종 승인자가 MR을 Merge할 때 Merge하지 않을 수 있는 시나리오:

  • 승인자가 승인 후 자동 Merge을 설정하는 것을 잊어버림.
  • 승인자가 자신이 마지막 승인자임을 인식하지 못함.
  • 승인자가 자동 Merge을 설정했지만 GitLab에서 설정이 해제됨.

이러한 시나리오 중 하나라도 발생하는 경우, MR 작성자는 모든 필요한 승인을 받았으며 리포지터리의 Merge 권한이 있는 경우 자신의 MR을 직접 Merge할 수 있습니다. 이는 또한 GitLab의 행동 우선 주의 가치와 일치합니다.

이 정책은 GitLab의 변경 관리 컨트롤인 CHG-04 제어를 충족시키기 위해 마련되었습니다.

gitlab-org/gitlab에서 이 정책을 적용하기 위해 다음과 같은 설정을 활성화했습니다.

gitlab-org/gitlab의 CODEOWNERS 파일에서 코드 소유자를 업데이트하려면, 코드 소유자 승인 핸드북 섹션에 설명된 프로세스를 따르십시오.

로컬에서 리베이스를 수행하거나 제안을 적용하는 등 일부 작업은 기존 승인을 초기화하는 것과 같이 간주됩니다. 승인은 UI에서 리베이스를 수행하거나 /rebase quick action를 사용하여 수행할 때 제거되지 않습니다.

Merge할 준비가 되면:

caution
Merge Request이 forked된 경우, 커뮤니티 기여를 위한 추가 지침도 확인하세요.
  • Merge Request에 커밋이 많은 경우 Squash and merge 기능을 사용하는 것이 좋습니다. 코드를 Merge할 때, Merge Request이 이미 이 옵션을 설정한 경우에만 유지보수자가 이 기능을 사용해야 하며, Merge Request에 뒤엉킨 커밋 히스토리가 명확히 드러나는 경우도 마찬가지입니다. 그렇지 않으면 몇 개의 커밋만 있는 MR의 경우, 유지보수자는 커밋을 squash하는 대신 작성자의 설정을 존중해야 합니다.
  • Merge Request의 Pipelines 탭으로 이동하여 Run pipeline을 선택한 후 Overview 탭에서 Auto-merge를 활성화하십시오. 다음 사항에 유의하십시오:
    • 기본 브랜치가 깨진 상태라면, Merge Request을 Merge하지 말아야 합니다. 특정 경우를 제외하고 handbook 설명을 따르십시오.
    • 최신 파이프라인이 승인된 후 생성되었으면, 전체 RSpec 스위트가 실행되었는지 확인하기 위해 새로운 파이프라인을 시작하십시오. Merge Request에 백엔드 변경 사항이 포함되지 않은 경우에만 이 단계를 건너뛸 수 있습니다.
    • 최신 Merge Result 파이프라인이 4시간 이내에 생성된 경우, 목표 브랜치에 도달하기가 충분히 가까운 상황이므로 새로운 파이프라인을 시작하지 않고 Merge할 수 있습니다.
  • MR을 자동 Merge으로 설정한 경우, 이후 발견되는 사항에 대한 재검토를 맡아야 합니다.
  • Squash and merge를 설정한 Merge Request의 경우, 스쿼시된 커밋의 기본 커밋 메시지는 Merge Request 제목에서 가져옵니다. Merge하기 전에 더 유익한 커밋 메시지를 가진 커밋을 선택하는 것이 좋습니다.

Merge Result 파이프라인 덕분에 작성자는 더 이상 자주(충돌이 발생할 때만) 브랜치를 리베이스할 필요가 없습니다. 이는 최종 사용자가 최종 리베이스를 요청받지 않아도 유지보수자가 MR 파이프라인을 시작하고 자동 Merge만 설정하면 되므로 리뷰/Merge 주기가 더 빨라집니다. 이 단계는 시점별로 테스트된 Merge Results를 이용하여 실제 Merge Train 기능에 아주 가까워지도록 합니다.

커뮤니티 기여

caution
악의적인 코드를 확인하기 위해 Merge된 결과 파이프라인을 시작하기 전에 모든 변경 사항을 면밀히 검토하십시오.

넓은 커뮤니티 기여자가 추가한 Merge Request을 검토할 때:

  • 루비 젬 및 노드 패키지와 같은 새로운 의존성 및 의존성 업데이트에 특별한 주의를 기울여야 합니다. Gemfile.lock 또는 yarn.lock과 같은 파일의 변경이 중요하지 않아 보일 수 있지만, 악의적인 패키지를 가져올 수도 있습니다.
  • 특히 문서 Merge Request의 링크와 이미지를 검토하십시오.
  • 의심스러울 때에는, Merge Request 파이프라인을 매뉴얼으로 시작하기 전에 @gitlab-com/gl-security/appsec의 누군가에게 Merge Request을 검토해 달라고 요청하십시오.
  • Merge Request이 현재 릴리즈에 포함될 가능성이 높을 때에만 마일스톤을 설정하십시오. 혼란을 피하고 아직 준비되지 않았을 때에는 마일스톤을 자주 옮기지 않도록 합니다.

만약 MR 소스 브랜치가 대상 브랜치보다 1,000개 이상의 커밋이 뒤처진 경우:

  • 저자에게 리베이스하거나, MR에 “대상 브랜치로 Merge할 수 있는 멤버의 커밋 허용” 기능이 활성화된 경우 자체적으로 리베이스하는 것을 고려해보십시오.
  • 최근 변경 내용을 고려하여 MR을 검토하면 숨은 런타임 충돌을 방지하고 일관성을 유지할 수 있습니다. 변경의 성격에 따라, 만약 MR이 1,000개 미만의 커밋이 뒤처진 경우 리베이스를 진행하는 것도 좋은 방법입니다.
  • 강제 푸시는 기여자에게 혼란을 야기할 수 있으므로, 리베이스를 실행했음을 알리는 것이 좋습니다. 또는 기여자가 MR에서 활발하게 작업 중일 때는 먼저 확인해볼 필요가 있습니다.
  • 리베이스는 보통 /rebase 빠른 조치로 GitLab 내에서 진행될 수 있습니다.

커뮤니티 Merge Request 인수

MR이 오랜 기간 동안 저자가 응답하지 않거나 MR을 완료할 수 없는 경우, GitLab은 이슈 및 Merge Request 마감 정책에 따라 인수할 수 있습니다. GitLab 엔지니어(일반적으로 MR 코치)는 다음을 진행할 것입니다:

  1. MR에 자신이 Merge하여 MR이 Merge될 수 있도록 인수하겠다는 댓글을 추가합니다.
  2. ~"coach will finish" 레이블을 해당 MR에 추가합니다.
  3. 메인 브랜치에서 새로운 기능 브랜치를 만듭니다.
  4. 그들의 브랜치를 새로운 기능 브랜치로 Merge합니다.
  5. 새로운 기능 브랜치를 메인 브랜치로 Merge하는 새 Merge Request을 엽니다.
  6. 커뮤니티 MR을 본인의 MR에서 연결하고 ~"Community contribution"로 레이블을 지정합니다.
  7. 필요한 최종 조정을 수행하고 변경 내용을 검토하도록 기여자에게 알림을 보내어 그들의 내용이 메인 브랜치로 Merge된다는 것을 인지하도록 합니다.
  8. 변경 사항이 Merge Request 지침을 준수하는지 확인합니다.
  9. Merge Request에 대한 보통 검토 프로세스를 따릅니다.

적절한 균형

코드 검토 중 가장 어려운 점 중 하나는 저자가 작성한 코드에 과도하게 개입할 수 있는 적절한 균형을 찾는 것입니다.

  • 적절한 균형을 찾는 데는 시간이 걸립니다. 그래서 일정 시간 이후에 유지자가 되는 리뷰어들이 있습니다.
  • 버그를 발견하는 것은 중요하지만, 좋은 설계에 대해 생각하는 것도 중요합니다. 추상화 및 좋은 설계를 구축하는 것이 복잡성을 숨기고 미래의 변경을 쉽게 만드는 것을 가능하게 합니다.
  • 코드 스타일의 강화와 향상은 주로 자동화를 통해, 리뷰 댓글보다 주로 해야 합니다.
  • 저자에게 설계를 변경하도록 요청하는 것은 기여한 코드의 완전한 재작성을 의미하기도 합니다. 이럴 때에는 일반적으로 다른 유지자 또는 리뷰어에게 먼저 물어보는 것이 좋지만, 중요하다고 믿을 때에는 용기 있게 실행하는 것이 좋습니다.
  • 반복의 이익을 위해, 귀하의 검토 제안이 블로킹되지 않는 변경이거나 개인적인 선호사항(문서화되지 않은 또는 합의된 요구사항이 아님)인 경우에는 저자에게 다시 전달하기 전에 Merge Request을 승인하는 것을 고려하십시오. 이는 저자가 동의하는 경우 귀하의 제안을 구현할 수 있도록 하거나, 유지자에게 바로 전달할 수 있도록 합니다. 이를 통해 전반적인 Merge 시간을 줄일 수 있습니다.
  • 일을 올바르게 하는 것과 지금 올바르게 하는 것에는 차이가 있습니다. 이상적으로는 전자를 따르는 것이 좋지만, 실제 세계에서는 후자가 필요합니다. 좋은 예로, 가능한 빨리 릴리스해야 하는 보안 수정이 있습니다. 긴급한 수정인 MR에서 주요한 리팩터링을 저자에게 요청하는 것을 피해야 합니다.
  • 오늘 일을 잘 하는 것이 내일 완벽하게 하는 것보다 일반적으로 더 나은 선택입니다. 지금의 방식으로 배포하는 것은 일반적으로 내일의 완벽한 방법보다 나쁩니다. 적절한 균형을 찾지 못할 때는 다른 사람들의 의견을 물어보십시오.

GitLab 특정 고려 사항

GitLab은 많은 곳에서 사용됩니다. 많은 사용자가 Omnibus 패키지를 사용하지만 일부 사용자는 도커 이미지를 사용하며, 일부는 소스에서 설치하며, 기타 설치 방법도 있습니다. GitLab.com 자체는 큰 엔터프라이즈 에디션 인스턴스입니다. 이에는 몇 가지 함의가 있습니다:

  1. 쿼리 변경은 GitLab.com 규모에서 성능이 악화되지 않도록 테스트해야 합니다:
    1. 지역적으로 대량의 데이터를 생성하는 것이 도움이 될 수 있습니다.
    2. GitLab.com으로부터 쿼리 계획을 요청하는 것이 가장 신뢰할 수 있는 방법입니다.
  2. 데이터베이스 마이그레이션은:
    1. 되돌릴 수 있어야 합니다.
    2. GitLab.com 규모에서 성능이 테스트되어야 합니다 - 확실하지 않다면 유지자에게 테스팅을 요청하십시오.
    3. 올바르게 분류되어야 합니다:
  3. Sidekiq worker는 역호환성을 유지할 수 없습니다:
    1. Sidekiq 큐는 배포 전에 드레인되지 않으므로 이전 버전의 GitLab에서 작업 중인 워커들이 있습니다.
    2. 메서드 시그니처를 변경해야 할 경우, 두 릴리스에 걸쳐 변경하고 처음에는 이전 및 새 인수를 모두 허용해야 합니다.
    3. 마찬가지로, 워커를 제거해야 할 경우, 첫 번째 릴리스에서 예약되지 않도록 하고 다음 릴리스에서 제거하십시오. 이를 통해 기존 작업이 실행됩니다.
    4. 또한, 모든 인스턴스가 모든 중간 버전으로 업그레이드되지 않음을 잊지 마십시오 (일부 사람들은 X.1.0에서 X.10.0으로, 또는 더 큰 업그레이드를 시도할 수도 있습니다!). 따라서, 비용이 적게든다면 이전 형식을 쉽게 허용하는 것이 좋습니다.
  4. 캐시된 값은 릴리스 간에 지속될 수 있습니다. 캐시 값의 반환 유형을 변경하는 경우 (예: 문자열 또는 nil에서 배열로), 동시에 캐시 키를 변경하십시오.
  5. 설정은 마지막 수단으로만 추가되어야 합니다. GitLab Rails에 새로운 설정 추가하기를 참조하십시오.
  6. 파일 시스템 액세스는 클라우드 네이티브 아키텍처에서는 불가능합니다. 수행해야 할 파일 저장에 대한 물체 저장 지원을 보장하십시오. 자세한 내용은 업로드 문서를 참조하십시오.

고객 중요 Merge Request

Merge Request이 고객 중요 우선순위로 고려될 경우 비즈니스에 상당한 이점이 있을 수 있습니다.

고객 중요 Merge Request의 속성:

  • 개발 부서의 시니어 디렉터 이상이 Merge Request이 고객 중요 사항으로 인정하는 것을 승인하여야 합니다. 또는, 직속 보고에게서 두 명이 승인할 경우에도 승인으로 간주됩니다.
  • DRI은 Merge Request에 customer-critical-merge-request 라벨을 적용하여야 합니다.
  • 고객 중요 Merge Request과 관련된 리뷰어 및 유지보수자가 이 결정이 내려진 즉시 참여하도록 필요로 합니다.
  • 고객 중요 Merge Request에 관여하는 사람들의 작업을 우선적으로 처리하여 해당 작업에 집중할 수 있는 충분한 시간을 확보하도록 필요로 합니다.
  • 고객 중요 Merge Request 작업 시 GitLab values 및 프로세스를 준수해야 하며, 특히 가족과 친구를 최우선, 작업 완료 정의, 반복, 그리고 사용 준비 시 릴리스에 특히 유의해야 합니다.
  • 고객 중요 Merge Request은 기술적인 결정에 우선순위를 정하는 과정의 절차에 따라 보안을 감소시키지 않고, 데이터 손실 위험을 도입하지 않고, 가용성을 감소시키지 않으며, 기존 기능을 손상시키지 않아야만 하는 요구사항을 가지고 있습니다.
  • 고객 중요 요청에 관해서는, 이를 Merge하는 데 소요되는 시간을 줄일 수 있을 것이라고 생각되는 경우 비동기(Merge Request의 코멘트)와 병행하여 동기적으로 조정하는 것을 권장 하며, 이는 이러한 절차가 효율성을 희생할 수 있음에도 불구하고 해당될 수 있습니다.
  • 고객 중요 Merge Request이 Merge된 후에는, 미래의 고객 중요 Merge Request을 빈도를 줄이기 위한 의도로 회고가 완료되어야만 하는 것을 필요로 합니다.

예시

코드 리뷰가 진행되는 방식은 새로운 기여자들을 놀라게 할 수 있습니다. 여기 몇 가지 코드 리뷰의 예시가 있어서, 기대할 것에 대한 방향성을 제시합니다.

“Modify DiffNote to reuse it for Designs”: 새 줄에 대한 작은 지적부터 디자인의 버전이 무엇이며, 이에 대해 어떻게 비교해야 하는지에 대한 이유 등 모든 것을 포함하고 있었습니다. 파일의 이전 버전이 없는 경우 특정 파일(부모 vs. 빈 sha vs. 빈 트리)의 경우를 논의하고 있습니다.

“Support multi-line suggestions”: 본 Merge Request 자체는 FE와 BE 간의 협업으로 구성되어 있으며, 작성자의 리뷰어에 대한 코멘트의 문서화가 포함되어 있습니다. 모든 면에 대한 소소한 지적과 정보 요청에 대한 일부 질문이 있으며, 끝부분에는 보안 취약점이 있습니다.

“Allow multiple repositories per project”: ZJ는 이것이 영향을 미칠 수 있는 다른 프로젝트 (workhorse)를 참조하며, 일관성을 위한 몇 가지 개선사항을 제안했습니다. 그리고 James의 코멘트가 위임 사용, &. 등 코드의 전반적인 품질 향상과 코드의 견고성을 높이는 데 도움이 되었습니다.

“Support multiple assignees for merge requests”: 코드베이스의 여러 부분에 영향을 미치는 MR에 대한 협업의 좋은 예시입니다. Nick는 흥미로운 에지 케이스를 지적했고, James Lopez도 가져오기/내보내기 기능에 대한 우려사항을 제기하여 참여했습니다.

출처

주로 thoughtbot 코드 리뷰 가이드를 기반으로 하고 있습니다.