Support default platform config in nerdctl.toml #1184
Conversation
| type RunConfig struct { | ||
| Platform string `toml:"platform"` | ||
| Network []string `toml:"network"` | ||
| } |
There was a problem hiding this comment.
Can we reuse the compose structure? https://pkg.go.dev/github.com/compose-spec/[email protected]/types#ServiceConfig
(Some fields such as image and container_name cannot be supported and should raise an error.)
There was a problem hiding this comment.
I think we should not reuse https://pkg.go.dev/github.com/compose-spec/[email protected]/types#ServiceConfig.
I think this would be a little bit complicated for user. For instance, like #1128, maybe people just need config like this
[default_run_config]
network=["test"]There was a problem hiding this comment.
I'm afraid we can't reuse the compose structure. I agree with @Zheaoli ’s option, the ServiceConfig is complicated for users. Another reason is that even if the parameters have the same name, they may not have the same type in different scenarios, e.g. platform only needs to be a string in the run command, but it needs to be a []string in the build command because it needs to support parsing parameters like arm64,amd64
There was a problem hiding this comment.
build
Platforms should be added as []string in the BuildConfig struct
https://pkg.go.dev/github.com/compose-spec/[email protected]/types#BuildConfig
There was a problem hiding this comment.
Hi @AkihiroSuda, I'd like to do some double-check to make sure I understand your suggestion correctly:
[default_run_config] will be decoded as ServiceConfig and [default_build_config] will be decoded as BuildConfig, right?
And another question,
(Some fields such as image and container_name cannot be supported and should raise an error.)
which means If the user adds these unsupported fields to the [default_run_config] section of the Nerdctl.toml file, we should raise an error when decoding?
There was a problem hiding this comment.
[default_run_config] will be decoded as ServiceConfig and [default_build_config] will be decoded as BuildConfig, right?
ServiceConfig contains BuildConfig, so no need to have [default_build_config].
A single default_template (per namespace) will suffice.
which means If the user adds these unsupported fields to the [default_run_config] section of the Nerdctl.toml file, we should raise an error when decoding?
Yes. At least a warning should be printed.
Like this: https://github.com/containerd/nerdctl/search?q=reflectutil.UnknownNonEmptyFields
There was a problem hiding this comment.
Linking BuildConfig.Platforms PRs
There was a problem hiding this comment.
Thank you for your patience and useful hints, I will follow the advice to complete the implementation
cmd/nerdctl/main.go
Outdated
| CgroupManager string `toml:"cgroup_manager"` | ||
| InsecureRegistry bool `toml:"insecure_registry"` | ||
| HostsDir []string `toml:"hosts_dir"` | ||
| RunConfig *RunConfig `toml:"default_run_config"` |
There was a problem hiding this comment.
Shouldn't this be map[string]... where the key is a containerd namespace string such as "default"?
There was a problem hiding this comment.
I think both map[string]... and user-defined structs are legal, I find an example from https://pkg.go.dev/github.com/pelletier/[email protected]#section-readme and test it works.
There was a problem hiding this comment.
Ah, if I can reuse the compose structure, then here I can just use map[namespace]ServiceConfig to avoid defining multiple similar structures. I'll have some try, thanks!
There was a problem hiding this comment.
I think it is more appropriate to choose parsing to structs than to maps here, so that only fields defined in structs will be parsed, and unsupported fields will be ignored in parsing even if they are mistakenly written to the configuration file by the user.
| type RunConfig struct { | ||
| Platform string `toml:"platform"` | ||
| Network []string `toml:"network"` | ||
| } |
There was a problem hiding this comment.
I think we should not reuse https://pkg.go.dev/github.com/compose-spec/[email protected]/types#ServiceConfig.
I think this would be a little bit complicated for user. For instance, like #1128, maybe people just need config like this
[default_run_config]
network=["test"]
|
@YangKian Is there any update about this PR? And you may need solve the conflict first |
|
@Zheaoli I'm sorry I've been a little busy at work lately and haven't had time to continue, but I should have more free time to continue writing starting next week. The ServiceConfig-based update is about 60% complete. I'll finish the rest as soon as I can, and then I think we can start the review first and finish the final update after the upstream pr merged. |
YangKian
force-pushed
the
platform
branch
2 times, most recently
from
ad0f6d9 to
7d70734
Compare
|
@YangKian Is there any update on this PR? |
|
I may take a hand on this PR base on what the owner has already done because the owner may not have enough time to do this if there is not any update on this PR before this weekend. cc @AkihiroSuda |
YangKian
force-pushed
the
platform
branch
3 times, most recently
from
dc08bdd to
d171092
Compare
Signed-off-by: YangKian <[email protected]>
|
Hey @Zheaoli is this something you would still like to engage on? |
|
@Zheaoli any update on this? ^ |
close #1154
Signed-off-by: Yang Qinyao [email protected]