Manjusaka

Manjusaka

What can we learn from a refactoring project?

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 PR (December 11, 2022). Currently, the refactoring progress has exceeded 80%, with contributions from over 6 contributors. This is definitely a significant undertaking.

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:

// Code snippet in Go

You can see that the logic in the apparmorLsAction function contains two parts:

  1. Flag handling (simple error handling)
  2. Command logic handling

This design has some obvious issues:

  1. Issues 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 also increases the learning curve for new contributors.
  2. The coupling of logic handling and flag handling makes it difficult to handle when the community tries to create a custom CLI scaffolding based on nerdctl. This can lead to complicated situations.

Nerdctl also has another issue. At the entry point of the cmd package, due to being in the same sub-package, internal helper functions were cross-referenced for convenience during the previous development process.

When nerdctl was initially created as an alternative to the containerd CLI, the design flaws were not obvious. However, when nerdctl started to provide a complete set of container lifecycle and network management features (based on CNI) and other advanced features (such as cosign, IPFS, etc.) as a de facto entry point for containerd, 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 album refactoring season - Nadeshiko Manju

Analysis of the Refactoring Process#

Alright, the community has a need, so I, Nadeshiko Manju, have to step up. Refactoring is simple, right? Just use GoLand and it's done. Easy peasy. So, I created a massive PR: Refactor the package structure in cmd/nerdctl nerdctl#1639. With over +5000 -4000 changes.

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

However, because this PR was too shocking, after I tested positive for COVID-19, Suda started to help me carry this PR. But in the end, even Suda couldn't carry it anymore (Suda's surprise: 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. And those two happinesses brought many more happinesses. I should have had a happy time like a dream... but why did it turn out like this... - "Nerdctl Diary"

In reality, the reason is quite simple. It was my mistake.

But let's get back on track. This PR is actually a textbook example of what not to do:

  1. There was no consensus within the community before starting such a large project.
  2. It violated the basic principle of "One PR for One Thing."
  3. There were too many unrelated changes during the refactoring, making the review process difficult.

So, after learning from the lessons of Refactor the package structure in cmd/nerdctl nerdctl#1639, I officially proposed a refactoring proposal to the community: Let's refactor the nerdctl CLI package nerdctl#1680. In this proposal, I did a few things:

  1. Clearly explained the necessity of the refactoring for future reference by community members.
  2. Defined several steps for the refactoring process.
  3. Established agreements for collaboration during the refactoring process.

The other maintainers in the community discussed some details and reached some consensus under this proposal:

  1. Narrowed down the final scope of the refactoring to only handle the flagging process.
  2. 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, I converted all these TODO tasks into separate issues to track the project's progress (which was like launching an email DDOS attack on the subscribers of this repo). I have to say, GitHub's project management tools are really weak (XDDDDD).

With two flowers blooming, each showing its own beauty, after the Proposal was officially accepted, the overall refactoring process started to pick up speed. Here are some interesting discussions that took place. Feel free to check them out if you're interested:

  1. Refactor the apparmor flagging process nerdctl#1774: A template PR after the Proposal was accepted, where further refinements were made to some details that were not fully addressed in the Proposal.
  2. [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 PR, discussing various parameter design styles.
  3. refactor: consolidate main logic of volume.List into volume.Volumes: Although it was not originally covered by the Proposal, the discussion about function semantic design in this PR is worth noting.
  4. pkg/cmd: inconsistent arguments ordering nerdctl#1889: A discussion about function design styles.

Of course, there are many other interesting discussions in the PRs, but I won't list them all here. Feel free to check out the original PRs (and join the discussion!).

Conclusion#

That's about it. I've summarized the gains and losses of the refactoring process so far. I hope you all enjoy it.

Loading...
Ownership of this post data is guaranteed by blockchain and smart contracts to the creator alone.