4
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

コインチェックAdvent Calendar 2024

Day 8

コードオーナーファイルのコードオーナーを設定せずに、コードオーナーの変更にコードオーナーの制限をかける方法について ~ CODEOWNERS を codeowners_owners/ に分割する ~

Last updated at Posted at 2024-12-07

この記事はコインチェック株式会社の アドベントカレンダー8日目(シリーズ1)の記事です。

TL;DR

  • GitHub では CODEOWNERS ファイルを利用して、コードオーナーを設定できる
  • CODEOWNERS ファイル自体の変更に細かい制限を設定 する方法は 2024/12 時点では存在しない
  • ブランチ保護ルールと CI で CODEOWNERS ファイルのフォーマットを制限した上で、分割されたファイルを利用することで 、 「コードオーナーの変更にコードオーナー自身のレビューを必須にする」 という制限をかけることができる
    • CODEOWNERS ファイルを編集した後、 codeowners_owners/ ディレクトリ下に自動生成ファイルを作る方式での具体的な実装(コード / 設定内容)を紹介する
    • 紹介した実装をそのまま利用することも可能 だが、 プロジェクト毎に最適化した方法を構築することを推奨 する
  • これによって、細かな統制と、開発のアジリティの両立 が実現できた

はじめに

こんにちは!
アプリケーション基盤部アプリケーション基盤グループ の 日下です。

コインチェックでは、 アドベントカレンダー4日目(シリーズ1)の Kaigi on Rails 2024に参加&登壇しました での登壇内容で紹介されているように、巨大なモノリシック なコードとなっていて、 packwerk を利用した機能のパッケージ化による分割を進めています。

モノリシック との戦いにおいては、packwerk の活用と並行して、 GitHub のコードオーナー機能も活用 しています。コードオーナー機能の活用に関しては、 「13,000 以上のソースファイル全てにコードのオーナーを割り振りを行った方法」など、今後紹介していきたい話もあるのですが、今回は ゲシュタルト崩壊気味のタイトルとともに、Tips を紹介したいと思います。

コードオーナーと統制

コードオーナーの設定

コードにオーナーを設定して管理する事は、コードベースと開発組織の拡大に伴い、必要性が増してきます。
オーナーが不明瞭なコードは、機能追加時や不具合発生時などの調査が、有識者を探して場当たり的な対応になりがちです。問題が根深い場合、根本的な解決は不可能です。
コードにオーナーを設定し、適切なレビュアーをアサインすることで、ナレッジが適切な場所に貯まり、対応の質が大幅に向上 するでしょう。

GitHub では、コードオーナー を設定できる機能があります。

GitHub のコードオーナー機能は、コードオーナーファイル (以下、CODEOWNERS ファイル) を利用して、ファイル単位で、オーナーを設定できます。

オーナーを設定しておくと、 GitHub 上でファイルを開いた時にそのファイルのオーナーを確認できることに加え、コードの変更提案(プルリクエスト作成)時に、 自動でレビュアーとしてアサインされます。

コードオーナーのレビューを必須にする

GitHub のブランチ保護は、リリースの統制において有益で、弊社でも活用しています。

GitHub の ブランチ保護の設定では、 コードオーナーのレビューを必須に設定 することができます。

レビューを必須にすることは、手続きを増やし、アジリティを下げる可能性があります。
そのため、対応の質を向上させるという側面に比べて、リリースを統制する意味合いが強くなります。

つまり、コードオーナーのレビューを必須にすることは、より細かいリリースの統制を実施することを意味します。

CODEOWNERS に対するオーナー設定

統制の観点では、コードのオーナーの定義の変更、つまり、 CODEOWNERS ファイルの変更時のレビューの設定 を考える必要があります。

つまり、コードオーナーに基づいたレビューを必須にしたとしても、 CODEOWNERS ファイルを自由に書き換え可能である場合、CODEOWNERS ファイルを変更することで、レビューの必須を解除することが可能 となります。

(もちろん、他の ブランチ保護ルールによって、不正なCODEOWNERS ファイルの変更は、運用で検知可能でしょう。ただし、他のブランチ保護ルールの承認者が、CODEOWNERS ファイルの変更内容を理解し、承認する権限を持つことになります)

そのため、GitHub のドキュメントにも、以下のような記載があります。

承認されていない変更からリポジトリを完全に保護するには、CODEOWNERS ファイル自体の所有者を定義する必要もあります。

CODEOWNERS ファイルに対して、コードオーナーを設定することは、 全てのコードオーナーの変更に対して責任を持つ承認者 が必要となり、これは、 開発のアジリティを更に下げる ことに繋がります。

CODEOWNERS ファイルに対して、コードオーナーを設定することで十分なケースもあるでしょう。例えばトップのディレクトリ単位でコードオーナーを設定し、その変更が頻繁に行われないケースなどです。

一方で、細かくコードオーナーを設定し、かつ、コードオーナーの変更が頻繁に発生する場合、CODEOWNERS ファイルに対してコードオーナーを設定して、開発のアジリティが下がるのを許容できないケースもあるでしょう。このケースについての対策を考えてみます。

コードオーナーの変更に、コードオーナー自身のレビューを必須にする

コードオーナーの変更により、アジリティが下がらないように、コードオーナーの変更に対するオーナーを新たに設定することは、再帰的になってしまい根本解決ではありません。

必要なことは、コードオーナー自身が 知らないうちにコードオーナーから外されないこと、つまり、 「コードオーナーの変更に、そのコードオーナー自身のレビューが必須である」 というアプローチが有効と考えました。

コードオーナーの変更に、コードオーナー自身のレビューを必須にするような機能は、2024/12 現在ではありません。

本記事では コードオーナー機能だけではなく、 CI (GitHub Actions)も利用して 、これを実現する方法を考えます。

コードオーナーの変更に、コードオーナー自身のレビューを必須にする方法

方式設計

方式

実現するために、以下を基本方針として仕組みを構築します。

  • GitHub の 機能を利用するため、CODEOWNERS をファイルを使用する。
    (独自の設定ファイル と WF などを作成せずに、GitHub の機能を活用する)
  • CODEOWNERS ファイル自身には、コードオーナーを設定しない。
    (全てのコードオーナーの変更に対する、承認者が必要になるため)
  • CODEOWNERS ファイルを分割した、コードオーナー毎のファイル を codeowners_owners/ ディレクトリの下に作成する。
    (コードオーナーは、ファイル単位での設定となるため)
    • GitHub がサポートしている CODEOWNERS ファイルをメンテ対象として、コードオーナー毎のファイルを作成する Script を用意する。
  • コードオーナー毎のファイル には、CODEOWNERS ファイルでオーナーを設定する
  • コードオーナー毎のファイル の内容と、CODEOWNERS ファイルの一致を担保する

この方針で、仕組みを実現するにはいくつかの注意点があります。

CODEOWNERS の文法と制限

CODEOWNERS ファイルの構文は .gitignore ファイルで使われている文法が使えます。

そのため、 * を利用したワイルドカード構文が利用できますが、ワイルドカード構文を利用すると、どのルールが有効なのかの判定が非常に難しくなるので、この利用を制限します。
(ワイルドカード構文を利用せずとも、フォルダ名を指定することでフォルダ単位でのコードオーナーの設定が可能です)

また、 CODEOWNERS ファイルでは、同一ルールが複数指定されていた場合、詳細度に関わらず 下側の定義が優先されるため、これも制限します。

CODEOWNERS
folder_1/a.txt @Alice
folder_1 @Bob # この定義で、 folder_1/a.txt についても オーナーが Bob になる

処理内容

上記から、 CODEOWNERS ファイルから コードオーナー毎のファイル を出力する Script の処理は、以下のようになるでしょう。

  • CODEOWNERS ファイルの解析
    • 各行を読み込み、 ファイルパス @owner1 @owner2 @owner3... # コメント という形式と想定して、パースする
    • ファイルパスについて、正規化( ./a/b/../c などを a/c に変換) と以下のチェックを行う
      • ワイルドカード( * ) が使用されていないこと
      • 同一パスが存在しないこと
      • 子 => 親の順で定義がされていないこと
    • コードオーナー毎のファイル定義が、CODEOWNERS ファイルに含まれていることの確認
    • コードオーナー毎のファイルの出力
      • codeowners_owners フォルダを作成し、その下に 「オーナー名.txt」でファイルを出力する
        • team の場合は「company_name/team_name」のようになるので、その場合は company_name のフォルダを作成する
        • ファイルは再作成(remove => create)とする。
      • ファイルの内容は、以下の2種類を想定します
        • 自身がオーナーの定義について、コメントを除いて ファイルパス @owner1 @owner2 @owner3 のように記載
          • 定義にオーナーのリストを含めることで、自身がオーナーのファイルのオーナーのメンバ変更に対してもチェックが可能となる
        • 自身がオーナーの定義の 子定義について、それが自身のオーナーではない場合 !ファイルパス という形式で記載
          • ディレクトリで指定されている場合、その子定義でオーバーライドしてオーナーを変更することを抑止するため

実装

スクリプト作成

ruby のスクリプトとして、 bin/generate_codeowners_owners.rb ファイルを以下の内容で用意します。
(本記事のコードは制約なしの利用を歓迎しますが、使用した結果に関する一切の責任を負いません。)

bin/generate_codeowners_owners.rb
#! /usr/bin/ruby
require "fileutils"

module GenerateCodeownersOwners
  module_function

  def main
    # CODEOWNERS ファイルの解析
    parsed_data, parse_errors = parse_file
    # コードオーナー毎のファイル定義が、CODEOWNERS ファイルに含まれていることの確認
    check_errors = check_exist_self(parsed_data)

    unless parse_errors.empty? && check_errors.empty?
      puts "CODEOWNERS ファイルの内容が不正です。以下のエラーの内容を修正してください。"
      (parse_errors + check_errors).each { |e| puts "[ERROR] #{e}" }
      exit(1)
    end

    # コードオーナー毎のファイルの出力
    recreate_codeowners_owners(parsed_data)

    puts "codeowners_owners/ 配下のファイルの再生成が正常に終了しました"
  end

  def parse_file
    parsed_data = {
      owner_hash: {},
      path_hash: {},
    }
    errors = []
    File.open(File.expand_path("../CODEOWNERS", __dir__)) do |f|
      f.each_line.with_index(1) do |line, index|
        next if line.match?(/^\s*($|#)/) # 空行,コメント行は無視

        valid_lin_splite = line.gsub(/\s*#.*$/, "").split(/\s+/) # 末尾のコメントを削除して分割

        path = File.expand_path(valid_lin_splite[0], "/")[1..] # パスの正規化
        if path.include?("*")
          errors.push "CODEOWNERS ファイルのパスに、ワイルドカード(*) が含まれています。 (#{index}行目)"
          next
        end
        if (same_path = parsed_data[:path_hash][path])
          errors.push "CODEOWNERS ファイルのパスに、重複があります。 (#{same_path[:index]}行目, #{index}行目)"
          next
        end
        if (child_path = parsed_data[:path_hash].find { |_, v| v[:path].start_with?("#{path}/") }&.dig(1))
          errors.push "CODEOWNERS ファイルのパスに、親ディレクトリによる上書きの定義があります。 (#{child_path[:index]}行目, #{index}行目)"
          next
        end

        owners = valid_lin_splite[1..]
        unless owners.all? { |o| o.start_with?("@") }
          errors.push "CODEOWNERS ファイルのオーナーに、'@' で始まらないオーナーが指定されています。 (#{index}行目)"
          next
        end
        owners.sort!.map! { |o| o[1..] } # 先頭の "@" を除去

        value = {
          path: path,
          index: index,
          owners: owners,
          children: [],
        }

        split_path = path.split("/")
        (split_path.size - 1).downto(1) do |i| # 親ディレクトリを探索
          parent_path = split_path[0...i].join("/")
          if parsed_data[:path_hash][parent_path]
            parsed_data[:path_hash][parent_path][:children].push value
            break
          end
        end

        owners.each do |o|
          parsed_data[:owner_hash][o] ||= []
          parsed_data[:owner_hash][o].push value
        end

        parsed_data[:path_hash][path] = value
      end
    end
    parsed_data[:owner_hash].each_value { |vs| vs.sort_by! { |v| v[:path] } }
    return parsed_data, errors
  end

  def check_exist_self(parsed_data)
    errors = []
    parsed_data[:owner_hash].each do |owner, values|
      unless values.any? { |v| v[:path] == codeowners_owner_path(owner) && v[:owners].size == 1 }
        errors.push "CODEOWNERS ファイルに '#{codeowners_owner_path(owner)} @#{owner}' の設定が必要です。"
      end
    end
    return errors
  end

  def recreate_codeowners_owners(parsed_data)
    codeowners_owners_dir = File.expand_path("../codeowners_owners", __dir__)
    if Dir.exist?(codeowners_owners_dir)
      FileUtils.rm_rf(codeowners_owners_dir)
    end

    parsed_data[:owner_hash].each do |owner, values|
      expand_codeowners_owner_path = File.expand_path("../#{codeowners_owner_path(owner)}", __dir__)

      # team の場合 @org/team-name のように、ディレクトリ構造となるためディレクトリを作成する
      FileUtils.mkdir_p(File.dirname(expand_codeowners_owner_path))
      File.open(expand_codeowners_owner_path, "w") do |f|
        values.each do |v|
          # codeowners_owner/ 下は CODEOWNERS 側でのオーナーの設定を含めてチェックしているため、不要
          next if v[:path] == codeowners_owner_path(owner)

          owners_str = v[:owners].map { |o| "@#{o}" }.join(" ")
          f.puts "#{v[:path]} #{owners_str}"
        end
        ignore_values = values.flat_map { |v| v[:children].filter { |c| !c[:owners].include?(owner) } }
        ignore_values.each do |v|
          f.puts "!#{v[:path]}"
        end
      end
    end
  end

  def codeowners_owner_path(owner)
    "codeowners_owners/#{owner}.txt"
  end
end

GenerateCodeownersOwners.main if __FILE__ == $0
GitHub Actions の WF 作成

上記で作成した bin/generate_codeowners_owners.rb を利用した、ワークフローを作成します。
( bin/generate_codeowners_owners.rb は、ワークフローのみでなく、コミット前に開発者が実行するためにも使います)

ワークフローで確認する内容は、以下の2点です

  • スクリプトで発生したエラーの確認
  • 自動生成ファイルの再作成後の差分の確認

以下の .yml ファイルを、 .github/workflows/codeowners_owners_check.yml に配置することで、 ワークフローが登録されます。

.github/workflows/codeowners_owners_check.yml
name: Codeowners owners check

on:
  pull_request:
    types: [opened, synchronize, reopened]

jobs:
  codeowners_owners_check:
    name: codeowners_owners_check
    runs-on: ubuntu-latest

    steps:
      - name: Checkout code
        uses: actions/checkout@v4

      - name: exec generate_codeowners_owners.rb
        run: ruby bin/generate_codeowners_owners.rb

      - name: check file diff
        run: |
          echo "以下の差分が発生している場合は、 `ruby bin/generate_codeowners_owners.rb` を実行して差分を解消してください。"
          git --no-pager diff && test -z "$(git status -s)"

ブランチ保護ルールの設定

Require review from Code Owners

ブランチの保護ルールの、「Require a pull request before merging」 を有効にし、その詳細項目中の「Require review from Code Owners」を有効にします。

これにより、コードオーナーが設定されているファイルについて、コードオーナーのレビューが必須となります

Require status checks to pass

ブランチの保護ルールの、「Require status checks to pass」 を有効にし、「 + Add checks 」 から 「codeowners_owners_check」を選択して、作成した GitHub Actions のチェックを有効にします。

これにより、 CODEOWNERS ファイルのチェックと、オーナー毎のコードオーナーが設定された codeowners/ 配下のファイルとの整合性が担保されます。

CODEOWNERS への設定

最後に、作成した bin/generate_codeowners_owners.rb.github/workflows/codeowners_owners_check.yml に コードオーナーを設定します。

これらのファイルを改竄すると、今回作成した仕組みが働かなくなることと、これらのファイルは頻繁に更新される必要がないため、 「ブランチ保護ルールの設定が可能」と同等程度の権限と考えて、コードオーナーを設定すると良いでしょう。

以下に、(統制として最低限必要な) CODEOWNERS ファイルのサンプルを記載します

CODEOWNERS
bin/generate_codeowners_owners.rb @org/maintain-team
.github/workflows/codeowners_owners_check.yml @org/maintain-team
codeowners_owners/org/maintain-team.txt @org/maintain-team

上記を記載して bin/generate_codeowners_owners.rb を動かすことで、 codeowners_owners/org/maintain-team.txt に以下の内容のファイルが作成されますので、この生成されたファイルと合わせて、更新してください。

codeowners_owners/org/maintain-team.txt
.github/workflows/codeowners_owners_check.yml @org/maintain-team
bin/generate_codeowners_owners.rb @org/maintain-team

動作確認

正常系

以下の通りに オーナーが既に設定されているとします。

CODEOWNERS
bin/generate_codeowners_owners.rb @org/maintain-team
.github/workflows/codeowners_owners_check.yml @org/maintain-team
codeowners_owners/org/maintain-team.txt @org/maintain-team

data/owner_a @owner_a
data/owner_a_or_b @owner_a @owner_b
data/owner_a_or_b/important.txt @owner_a

codeowners_owners/owner_a.txt @owner_a
codeowners_owners/owner_b.txt @owner_b
codeowners_owners/owner_a.txt
data/owner_a @owner_a
data/owner_a_or_b @owner_a @owner_b
data/owner_a_or_b/important.txt @owner_a
codeowners_owners/owner_b.txt
data/owner_a_or_b @owner_a @owner_b
!data/owner_a_or_b/important.txt

ここで、 CODEOWNERS ファイルに記載されている、 important.txt のオーナーを変更します。

CODEOWNERS(diff)
data/owner_a_or_b @owner_a @owner_b
-data/owner_a_or_b/important.txt @owner_a
+data/owner_a_or_b/important.txt @owner_b

./bin/generate_codeowners_owners.rb を実行して、差分を確認します

codeowners_owners/owner_a.txt(diff)
 data/owner_a_or_b @owner_a @owner_b
-data/owner_a_or_b/important.txt @owner_a
+!data/owner_a_or_b/important.txt
codeowners_owners/owner_b.txt(diff)
 data/owner_a_or_b @owner_a @owner_b
-!data/owner_a_or_b/important.txt
+data/owner_a_or_b/important.txt @owner_b

codeowners_owners/owner_a.txt と codeowners_owners/owner_b.txt に 差分が発生しています。

この変更に対する PR を作成します。

( ※ 以下、画像は Reviewers の部分など一部マスクしたものとなります。 )

PR 作成画面において、コードオーナーのレビューが必要なことが示されます。

(作成後)

image-20241204-005433 (1).png

設定した「Require review from Code Owners」により、Code owner の レビューが必要なため、 approve がつくまで merge が抑制されます

image.png

コマンド実行結果は割愛しますが、以下のようなケースで適切なオーナーへのレビューが必須になることが確認できます

  • A が owner の設定を追加した場合
    • A のレビューが必須となる
  • owner を A → B に変更した場合
    • A と B のレビューが必須となる
  • A と B が owner のファイルのオーナーに、 C を追加した場合
    • A と B と C のレビューが必須となる
  • A が owner の設定を削除した場合
    • A のレビューが必須となる
  • A が owner のフォルダの配下の、B が owner のファイルのオーナーを C に変更した場合
    • B と C のレビューが必須になる
    • A のレビューは不要

ただし、上記には 1点、条件があります。
GitHub のドキュメントには以下の記載があります。

コードオーナーがレビューのリクエストを受け取るためには、CODEOWNERS ファイルがプルリクエストの base ブランチになければなりません。

これは、PR でコードオーナーを追加するときに、既に追加されているコードオーナーに対してのみ、レビューのリクエストが発生する、と読み替えられます。

つまり、既にオーナーとなっているファイルが存在する ( codeowners_owners/owner_a.txt が存在する)場合には、上記の条件の通り動くのですが、まだ、オーナーとなっているファイルが存在していないオーナーに対しての、最初のPR においては、追加時のレビューは必須とはなりません。

コードオーナーの変更 / 削除時は、制限を変更するため、統制上の観点からレビュー必須が大事になりますが、追加時は制限を強化するだけという理由と、運用などで回避も可能なので、この記事としては、これについての対応は対象外とします。

異常系

CODEOWNERS ファイルの定義がおかしい以下のファイルを作成します

CODEOWNERS
bin/generate_codeowners_owners.rb @org/maintain-team
.github/workflows/codeowners_owners_check.yml @org/maintain-team
codeowners_owners/org/maintain-team.txt @org/maintain-team

data/owner_a @owner_a
data/owner_a_or_b/owner_a.txt @owner_a
data/owner_a_or_b @owner_a @owner_b
data/owner_b owner_b # コメント
data/owner_c @owner_c
data/team_a/* @org/team_a
data/team_a/owner_b.txt @owner_b
data/team_a/owner_b.txt @owner_a @owner_b
data/common/owner_c @owner_c

codeowners_owners/owner_a.txt @owner_a
codeowners_owners/owner_b.txt @owner_b
codeowners_owners/owner_c.txt @owner_c
codeowners_owners/org/team_a.txt @org/team_a

CI の結果が不正となり、 merge が抑制されます

image-20241201-033053.png

Details をクリックすることで、CODEOWNERS のエラー内容が確認できます

image-20241201-105244.png

また、別のケースとしてCODEOWNERS ファイルだけを更新して、 bin/generate_codeowners_owners.rb を動かさなケースを考えます。

差分が発生するため、 CI の結果が不正となり、 merge が抑制されます

image-20241201-110022.png

Details をクリックすることで、 bin/generate_codeowners_owners.rb の実行が必要であることに気づくことができます。

image-20241204-085520.png

おわりに

この記事で紹介した「コードオーナーの変更に、コードオーナー自身のレビューを必須にする方法」は、コインチェックの業務で、そのまま使っているわけではありません。

というのも、コインチェックでは以下のような前提があります。

  • CODEOWNERS ファイル自体をツールで生成しているため、この記事で書いたフォーマットチェックが不要
  • コードオーナーには、原則として Team を設定するが、通知などの用途で合わせて Team 内の個人を設定することも可能としていて、コードオーナーの変更レビューは Team のみに設定したい

つまり、「仕組み自体入れているが、 実装は一部異なる 」といえます。

また、この「コードオーナーの変更に、コードオーナー自身のレビューを必須にする」という制約は、数あるブランチ保護ルールの一つ であり、統制のためのフィルターの一部でしかありません。

そのため、ソースファイル全てにコードのオーナーを割り振りを行ったのですが、CODEOWNERS ファイルに設定を入れて、レビュー必須の対象とするものは一部のファイルに限っている、という事情もあります。
(一律ではなく、GitHub の CODEOWNERS ファイル側に レビュー必須の有無を設定できるように、GitHub 側の機能が拡充されることを、密かに望んでいます)

この記事は、実際に今年取り入れた仕組みの紹介する、という点はあるものの、「コードオーナーに対するコードオーナー」という再帰的ともいえるテーマが楽しいかな?という動機もありました。
結果として、コードにオーナーを設定するコードオーナーの仕組み」と「不正なコードが入らないための、ブランチ保護ルールの仕組み」を組み合わせるという、やや発展的で、モノリス かつ (金融系など) 厳しい統制が必要な場合の、ニッチな領域の話になりました。

読み物として楽しんでいただき、何かの参考になれば幸いです。

コインチェックのアドベントカレンダーは、まだまだ続きますのでお楽しみに!

4
0
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
4
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?