Migrate TOML to github.com/pelletier/go-toml#5278
Migrate TOML to github.com/pelletier/go-toml#5278dmcgowan merged 7 commits intocontainerd:masterfrom
Conversation
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
|
Added a workaround to keep structures private in docker hosts parser package (current pelletier/go-toml implementation requires fields to be public in order to be deserialized). |
|
Build succeeded.
|
|
Do we need to put this commit in the release/1.5? |
|
can wait if you have concerns. |
|
The change seems significant enough for it to be desirable to get it into 1.5 just for the sake of backporting potential bug fixes. |
|
Either that or waiting until well after 1.5 is cut. |
remotes/docker/config/hosts.go
Outdated
| "github.com/BurntSushi/toml" | ||
| "github.com/pelletier/go-toml" | ||
| "github.com/pkg/errors" | ||
|
|
There was a problem hiding this comment.
Looks like imports were accidentally split here
remotes/docker/config/hosts_test.go
Outdated
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
|
|
| if err != nil { | ||
| return nil, err | ||
| } | ||
| hosts = append(hosts, parsed) |
There was a problem hiding this comment.
The ordering here is important and I don't see where it is preserved. Is that part of the test coverage?
| // Options are config options for the runtime. If options is loaded | ||
| // from toml config, it will be toml.Primitive. | ||
| Options *toml.Primitive `toml:"options" json:"options"` | ||
| Options *toml.Tree `toml:"options" json:"options"` |
There was a problem hiding this comment.
nit: comment also needs to be updated
Signed-off-by: Maksym Pavlenko <[email protected]>
|
Addressed feedback and restored ordering |
|
Build succeeded.
|
containerd made this change in containerd/containerd#5278, we need to do the same in our cri fork. Signed-off-by: Amit Barve <[email protected]>
9 participants
https://github.com/BurntSushi/toml is officially deprecated (BurntSushi/toml@ea60c4d), so this PR migrates containerd to https://github.com/pelletier/go-toml.
Docker hosts parser had to be rewritten as it was tied to TOML package implementation.
Also the original implementation respected host order defined in the file, the current one is not (because pelletier/go-toml uses
mapinternally to keep keys). Other than that the behavior should be the same. @dmcgowan could you please TAL d56b49c?Fixes: #4370