Skip to content

Replace BurntSushi/toml with pelletier/go-toml#42246

Merged
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:replace_toml
Apr 8, 2021
Merged

Replace BurntSushi/toml with pelletier/go-toml#42246
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:replace_toml

Conversation

@thaJeztah
Copy link
Member

relates to containerd/containerd#5278
I'll also have a look at replacing the remaining uses in libnetwork

validate/toml: switch to github.com/pelletier/go-toml

The github.com/BurntSushi/toml project is no longer maintained,
and containerd is switching to this project instead, so start
moving our code as well.

This patch only changes the binary used during validation (tbh,
we could probably remove this validation step, but leaving that
for now).

I manually verified that the hack/verify/toml still works by adding a commit
that makes the MAINTAINERS file invalid;

diff --git a/MAINTAINERS b/MAINTAINERS
index b739e7e20c..81ababd8de 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23,7 +23,7 @@
        # a subsystem, they are responsible for doing so and holding the
        # subsystem maintainers accountable. If ownership is unclear, they are the de facto owners.

-               people = [
+               people =
                        "akihirosuda",
                        "anusha",
                        "coolljt0725",

Running hack/verify/toml was able to detect the broken format;

    hack/validate/toml
    (27, 4): keys cannot contain , characterThese files are not valid TOML:
     - MAINTAINERS

    Please reformat the above files as valid TOML

libcontainerd/supervisor: replace BurntSushi/toml with pelletier/go-toml

Taking the same approach as was taken in containerd

The new library has a slightly different output;

  • keys at the same level are sorted alphabetically
  • empty sections not omitted (proxy_plugins, stream_processors, timeouts),
    which could possibly be be addressed with an "omitempty" in containerd's struct.
  • empty slices are not omitted (imports, required_plugins)

After sorting the "before" configuration the diff looks like this:

diff --git a/config-before-sorted.toml b/config-after.toml
index cc771ce7ab..43a727f589 100644
--- a/config-before-sorted.toml
+++ b/config-after.toml
@@ -1,6 +1,8 @@
 disabled_plugins = ["cri"]
+imports = []
 oom_score = 0
 plugin_dir = ""
+required_plugins = []
 root = "/var/lib/docker/containerd/daemon"
 state = "/var/run/docker/containerd/daemon"
 version = 0
@@ -37,6 +39,12 @@ version = 0
     shim = "containerd-shim"
     shim_debug = true

+[proxy_plugins]
+
+[stream_processors]
+
+[timeouts]
+
 [ttrpc]
   address = ""
   gid = 0

Before and after config-files are attached in the zip below:

config-files.zip

The github.com/BurntSushi/toml project is no longer maintained,
and containerd is switching to this project instead, so start
moving our code as well.

This patch only changes the binary used during validation (tbh,
we could probably remove this validation step, but leaving that
for now).

I manually verified that the hack/verify/toml still works by adding a commit
that makes the MAINTAINERS file invalid;

        diff --git a/MAINTAINERS b/MAINTAINERS
        index b739e7e..81ababd8de 100644
        --- a/MAINTAINERS
        +++ b/MAINTAINERS
        @@ -23,7 +23,7 @@
                # a subsystem, they are responsible for doing so and holding the
                # subsystem maintainers accountable. If ownership is unclear, they are the de facto owners.

        -               people = [
        +               people =
                                "akihirosuda",
                                "anusha",
                                "coolljt0725",

Running `hack/verify/toml` was able to detect the broken format;

        hack/validate/toml
        (27, 4): keys cannot contain , characterThese files are not valid TOML:
         - MAINTAINERS

        Please reformat the above files as valid TOML

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Taking the same approach as was taken in containerd

The new library has a slightly different output;

- keys at the same level are sorted alphabetically
- empty sections not omitted (`proxy_plugins`, `stream_processors`, `timeouts`),
  which could possibly be be addressed with an "omitempty" in containerd's struct.
- empty slices are not omitted (`imports`, `required_plugins`)

After sorting the "before" configuration the diff looks like this:

```patch
diff --git a/config-before-sorted.toml b/config-after.toml
index cc771ce7ab..43a727f589 100644
--- a/config-before-sorted.toml
+++ b/config-after.toml
@@ -1,6 +1,8 @@
 disabled_plugins = ["cri"]
+imports = []
 oom_score = 0
 plugin_dir = ""
+required_plugins = []
 root = "/var/lib/docker/containerd/daemon"
 state = "/var/run/docker/containerd/daemon"
 version = 0
@@ -37,6 +39,12 @@ version = 0
     shim = "containerd-shim"
     shim_debug = true

+[proxy_plugins]
+
+[stream_processors]
+
+[timeouts]
+
 [ttrpc]
   address = ""
   gid = 0
```

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah requested a review from tianon as a code owner April 2, 2021 15:49
@thaJeztah thaJeztah added kind/refactor PR's that refactor, or clean-up code status/2-code-review labels Apr 2, 2021
@thaJeztah thaJeztah changed the title Replace replace BurntSushi/toml with pelletier/go-toml Replace BurntSushi/toml with pelletier/go-toml Apr 2, 2021
@thaJeztah
Copy link
Member Author

@cpuguy83 @tiborvass ptal

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit 1cb7ee4 into moby:master Apr 8, 2021
@thaJeztah thaJeztah deleted the replace_toml branch April 8, 2021 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants