-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate TOML to github.com/pelletier/go-toml #5278
Conversation
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like imports were accidentally split here
remotes/docker/config/hosts_test.go
Outdated
@@ -26,6 +26,8 @@ import ( | |||
"path/filepath" | |||
"testing" | |||
|
|||
"github.com/stretchr/testify/assert" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
} | ||
hosts = append(hosts, parsed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ordering here is important and I don't see where it is preserved. Is that part of the test coverage?
@@ -49,7 +49,7 @@ type Runtime struct { | |||
Root string `toml:"runtime_root" json:"runtimeRoot"` | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comment also needs to be updated
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Addressed feedback and restored ordering |
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
containerd made this change in containerd/containerd#5278, we need to do the same in our cri fork. Signed-off-by: Amit Barve <ambarve@microsoft.com>
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
map
internally to keep keys). Other than that the behavior should be the same. @dmcgowan could you please TAL d56b49c?Fixes: #4370