Originally, this article was supposed to be completed before the last working day of 2022, but due to procrastination, it is only finished now. However, I still want to share it with you all, hoping that the content inside can be helpful.
Background Introduction#
This refactoring project has been going on for about a month and a half since my first major refactoring pull request (December 11, 22). Currently, the refactoring progress has exceeded 80%, with contributions from more than 6 contributors. This is definitely a significant engineering effort.
Now, the question is, why did I initiate this refactoring project?
Before the refactoring project, the nerdctl project had a major issue at the entry point of commands, which was the coupling of flag handling and logic. For example, let's take the nerdctl apparmor series of code as an example:
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: "List the loaded AppArmor profiles",
Args: cobra.NoArgs,
RunE: apparmorLsAction,
SilenceUsage: true,
SilenceErrors: true,
}
cmd.Flags().BoolP("quiet", "q", false, "Only display profile names")
// Alias "-f" is reserved for "--filter"
cmd.Flags().String("format", "", "Format the output using the given go template")
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("unsupported format: \"raw\"")
default:
if quiet {
return errors.New("format and quiet must not be specified together")
}
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
}
You can see that the logic in the apparmorLsAction function includes two parts:
- Flag handling (simple error handling)
- Command logic handling
This design has obvious issues:
- Problems with code readability and maintainability. For example, when I need to add a flag, I have to add it in multiple places. The excessive flagging process makes it difficult for new contributors to enter the project.
- The coupling of logic handling and flag handling leads to difficulties when the community tries to wrap a custom CLI scaffold based on nerdctl. It becomes very challenging.
Nerdctl also has another issue. At the entry point of the cmd package, due to being in the same sub-package, during the previous development process, the internal helper functions of each file were cross-referenced for convenience.
When nerdctl was initially just a replacement for the containerd CLI, the design flaws were not very apparent. However, when nerdctl became a complete entry standard for containerd, providing a set of container lifecycle and network management features (based on CNI) and other advanced features (such as cosign, IPFS, etc.), the community undoubtedly had higher demands. For example, Move *.go files for subcommand out main package nerdctl#1631 is a typical example.
In this situation, it became necessary and urgent to perform a reasonable but extensive refactoring of the entry point of nerdctl.
It's time for another
white albumrefactoring season - Nadeshiko Manju
Refactoring Process Analysis#
Alright, the community has a need, so I, Saka, no, Nadeshiko Manju, have to step up. Refactoring is simple, just use GoLand to do it, right? Easy peasy. So, I created a massive pull request: Refactor the package structure in cmd/nerdctl nerdctl#1639. Size: +5000 -4000.
However, because this pull request was too shocking, after I tested positive for COVID-19, Suda started helping me carry this pull request. But in the end, even Suda couldn't carry it (Suda is shocked: Baka saka!)
Why did it turn out like this... For the first time, I had the desire to refactor, and there was a need for refactoring. Two happy things overlapped. The happiness of these two things brought me a lot of happiness. I should have had a happy time like a dream... But why did it turn out like this... - "Nerdctl Photo Album"
Actually, the reason is quite simple Fuyuhara Kōsuke, oh no, it's me, Kōsuke. Oh, no, it's just that my brain got stuck in the door.
Anyway, back to the point, this pull request is actually a textbook example of what not to do:
- There was no consensus in the community before starting such a large-scale project.
- It violated the basic principle of "One PR for One Thing."
- There were too many unrelated changes during the refactoring, making the review process extremely difficult.
So, after learning from the lessons of Refactor the package structure in cmd/nerdctl nerdctl#1639, I officially proposed a refactoring proposal in the community: Let's refactor the nerdctl CLI package nerdctl#1680. In this proposal, I did a few things:
- Clearly explained the necessity of the refactoring for future reference by community members.
- Defined several steps for the refactoring process.
- Established agreements for collaboration among multiple contributors during the refactoring.
The other maintainers in the community had additional discussions and reached some consensus under this proposal:
- Narrowed down the final scope of the refactoring to only handle the flagging process.
- Optimized the design of some file structures.
Up until now, the refactoring of nerdctl has finally started to enter a fast lane. After all, refactoring is not about writing randomly. If you make a mistake, you have to apologize to the community.
There was also an interesting incident. Initially, after creating TODO tasks in the issue, to track the progress of the project, I directly converted all these TODO tasks into issues (which was like launching an email DDOS attack on the subscribers of this repo). I have to complain a bit here, GitHub's project management tools are really weak (XDDDDD).
With two flowers blooming, each expressing one, after the proposal was officially accepted, the overall refactoring process began to enter the fast lane. Here are some interesting discussions. Feel free to check them out if you're interested:
- Refactor the apparmor flagging process nerdctl#1774: A template pull request after the proposal was accepted, this pull request further refined some details that were not well addressed in the proposal.
- [Refactor] Refactor the build subcommand flagging process nerdctl#1792: The first major refactoring of a command after the proposal was accepted, in a sense, it also became a template pull request. It discussed various parameter design styles.
- refactor: consolidate main logic of volume.List into volume.Volumes: Although it was not originally covered by the proposal, it is worth noting the discussion about function semantic design.
- pkg/cmd: inconsistent arguments ordering nerdctl#1889: A discussion about function design style.
Of course, there are many interesting discussions in other pull requests as well, but I won't list them all here. Feel free to check out the original pull requests (and join the discussions if you'd like).
Conclusion#
That's about it. I've summarized the gains and losses during the refactoring process so far. I hope you all enjoy it.