リファクタリングプロジェクトから何を学べるか

本来この文章は 2022 年の最後の勤務日前に書き終える予定でしたが、先延ばしにしてしまい、今になってやっと書き終えました。それでも公開しますので、内容が皆さんの助けになれば幸いです。

背景紹介#

このリファクタリングプロジェクトは、私の最初の超大型リファクタリング PR（22 年 12 月 11 日）から数えて、すでに 1 ヶ月半が経過しました。現在、リファクタリングの進捗は 80％を超え、6 人以上の貢献者が集まって貢献しています。これは決して小さなプロジェクトではありません。

さて、問題です。なぜ私はこのリファクタリングプロジェクトを立ち上げたのでしょうか？

リファクタリングプロジェクトの前、nerdctl プロジェクトには大きな問題がありました。それは、コマンドのエントリポイントにおけるフラグの処理とロジックの結合の問題です。例えば、nerdctl apparmorシリーズのコードを例に挙げてみましょう。

package main

import (
	"bytes"
	"errors"
	"fmt"
	"text/tabwriter"
	"text/template"

	"github.com/containerd/nerdctl/pkg/apparmorutil"
	"github.com/spf13/cobra"
)

func newApparmorLsCommand() *cobra.Command {
	cmd := &cobra.Command{
		Use:           "ls",
		Aliases:       []string{"list"},
		Short:         "読み込まれたAppArmorプロファイルをリスト表示する",
		Args:          cobra.NoArgs,
		RunE:          apparmorLsAction,
		SilenceUsage:  true,
		SilenceErrors: true,
	}
	cmd.Flags().BoolP("quiet", "q", false, "プロファイル名のみを表示する")
	// "-f"のエイリアスは"--filter"のために予約されています
	cmd.Flags().String("format", "", "指定されたGoテンプレートを使用して出力をフォーマットする")
	cmd.RegisterFlagCompletionFunc("format", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return []string{"json", "table", "wide"}, cobra.ShellCompDirectiveNoFileComp
	})
	return cmd
}

func apparmorLsAction(cmd *cobra.Command, args []string) error {
	quiet, err := cmd.Flags().GetBool("quiet")
	if err != nil {
		return err
	}
	w := cmd.OutOrStdout()
	var tmpl *template.Template
	format, err := cmd.Flags().GetString("format")
	if err != nil {
		return err
	}
	switch format {
	case "", "table", "wide":
		w = tabwriter.NewWriter(cmd.OutOrStdout(), 4, 8, 4, ' ', 0)
		if !quiet {
			fmt.Fprintln(w, "NAME\tMODE")
		}
	case "raw":
		return errors.New("サポートされていないフォーマット: \"raw\"")
	default:
		if quiet {
			return errors.New("formatとquietは同時に指定できません")
		}
		var err error
		tmpl, err = parseTemplate(format)
		if err != nil {
			return err
		}
	}

	profiles, err := apparmorutil.Profiles()
	if err != nil {
		return err
	}

	for _, f := range profiles {
		if tmpl != nil {
			var b bytes.Buffer
			if err := tmpl.Execute(&b, f); err != nil {
				return err
			}
			if _, err = fmt.Fprintf(w, b.String()+"\n"); err != nil {
				return err
			}
		} else if quiet {
			fmt.Fprintln(w, f.Name)
		} else {
			fmt.Fprintf(w, "%s\t%s\n", f.Name, f.Mode)
		}
	}
	if f, ok := w.(Flusher); ok {
		return f.Flush()
	}
	return nil
}

apparmorLsAction関数のロジックには、2 つの部分が含まれています。

フラグの処理（シンプルなエラーハンドリング（XDDDDD）
コマンドロジックの処理

このような設計には明らかな問題があります。

コードの可読性と保守性の問題。例えば、フラグを追加する必要がある場合、複数の場所に追加する必要があります。また、フラグの処理が多すぎると、新人がプロジェクトに参加する際のハードルが上がります。
ロジックの処理とフラグの処理が結合しているため、コミュニティが nerdctl を基にカスタム CLI フレームワークを構築しようとすると、非常に扱いにくい状況が発生します。

さらに、nerdctl には別の問題もあります。コマンドのエントリポイントで、同じサブパッケージに属しているため、以前の開発過程で手間を省くために、ファイル間でお互いの内部ヘルパー関数を交差参照していました。

nerdctl プロジェクトが最初に containerd CLI の代替品としてのみ存在していたとき、以前の設計上の欠陥は実際には明らかではありませんでした。しかし、nerdctl が containerd に基づくコンテナライフサイクルとネットワーク管理（CNI に基づく）およびその他の高度な機能（cosign、IPFS など）を完全に提供し、containerd の実質的なエントリスタンダードとして機能し始めたとき、コミュニティは明らかにより高い要求を持つようになります。例えば、Move *.go files for subcommand out main package nerdctl#1631は非常に典型的な例です。

このような状況下で、nerdctl のエントリポイントを合理的かつ大規模にリファクタリングすることは、必須かつ緊急の課題となりました。

また~~白色相薄~~リファクタリングの季節がやってきました --- 蛮久抚子（Nadeshiko Manju）

リファクタリングプロセスの分析#

さて、コミュニティのニーズがあるので、saka、ああ、蛮久抚子（Nadeshiko Manju）が立ち上がらなければなりません。リファクタリングは簡単です、Goland でちょっとやれば終わりですから。そう言って、私は超大規模な PR を作成しました：Refactor the package structure in cmd/nerdctl nerdctl#1639。規模は + 5000 -4000 です。

よし、気合いが勝っとる！

しかし、この PR はあまりにも衝撃的だったため、私が COVID-19 陽性になった後、Suda がこの PR を手伝い始めました。しかし、最終的に Suda も「持ちきれない」と叫びました（Suda の驚き：ばか saka！）

どうしてこうなるんだろう… 初めて、リファクタリングしたいという欲求があり、リファクタリングの必要性がありました。嬉しいことが二つ重なって。その二つの嬉しさが、また、たくさんの嬉しさを連れてきてくれて。夢のように幸せな時間を手に入れたはずなのに… なのに、どうして、こうなっちゃうんだろう…
どうしてこうなったのか、初めてリファクタリングしたいという欲求があり、リファクタリングの必要性がありました。二つの嬉しいことが重なり、その二つの嬉しさがさらに多くの嬉しさをもたらしました。本来は夢のように幸せな時間を得るはずだったのに… なぜ、こうなってしまったのか…… ---- 《nerdctl 相薄》

実際のところ、理由は非常にシンプルです ~~冬馬小三~~ 、ああ、私の小三、ああ、違う、私の頭がドアに挟まれたのです。

本題に戻りますが、実際、この PR は教科書的な反面教師の例です。

大規模プロジェクトを開始する前にコミュニティの合意を得ていない
One PR for One Thing の基本原則に反している
リファクタリング時の無関係な変更が多すぎて、レビューが非常に困難になっている

したがって、Refactor the package structure in cmd/nerdctl nerdctl#1639の教訓を踏まえて、私は正式にコミュニティにリファクタリング提案を提出しました：Let's refactor the nerdctl CLI package nerdctl#1680。この提案では、いくつかのことを行いました。

リファクタリングの必要性を完全に説明し、コミュニティメンバーが後で振り返るのを容易にする
リファクタリングのいくつかのステップを定義する
複数人で協力してリファクタリングを行う際の共通の合意を定める

コミュニティの他のメンテナーたちは、この提案の下で追加の詳細を議論し、いくつかの合意に達しました。

最終的なリファクタリングの範囲をフラグ処理のみに縮小する
一部のファイル構造の設計を最適化する

現在までに、nerdctl のリファクタリングは正式に加速し始めました。結局、リファクタリングは無闇に行うものではなく、間違ったことを書いた場合はコミュニティに謝罪しなければなりません。

ここで実はちょっとしたエピソードがあります。最初に Issue で TODO タスクを作成した後、プロジェクトの進捗を追跡するために、これらの TODO タスクをすべて Issue に変換しました（そして、実質的にこのリポジトリを購読している人たちにメールの DDOS をかけることになりました）。ここで一言言わせてください。GitHub のプロジェクト管理ツールは本当に弱いです（XDDDDD

花開二朵、それぞれ一つずつ、提案が正式に承認された後、全体のリファクタリングは加速し始めました。ここでいくつかの興味深い議論を挙げておきますので、興味がある方はぜひ見てみてください。

Refactor the apparmor flagging process nerdctl#1774、提案受理後のテンプレート PR で、この PR の下で提案の中で議論されていなかった詳細をさらに細分化しました。
[Refactor] Refactor the build subcommand flagging process nerdctl#1792、提案受理後の最初の大きなコマンドのリファクタリングで、ある意味ではテンプレート PR でもあり、パラメータ設計スタイルに関する多くの問題が議論されました。
refactor: consolidate main logic of volume.List into volume.Volumes、提案の範囲には含まれませんが、関数の意味設計に関する議論は注目に値します。
pkg/cmd: inconsistent arguments ordering nerdctl#1889、関数設計スタイルに関する問題です。

もちろん、他にも多くの PR の議論が非常に興味深いものですので、ここではすべてを挙げることはできません。ぜひ原文の PR を直接見て（もちろん議論に参加することも歓迎です）。

まとめ#

だいたいこんな感じで、これまでのリファクタリングプロセスの得失を振り返ってみました。皆さんに楽しんでもらえれば幸いです。