-
Notifications
You must be signed in to change notification settings - Fork 142
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
generate: output minimal template in default #379
base: master
Are you sure you want to change the base?
Conversation
On Wed, May 10, 2017 at 07:36:08PM -0700, Ma Shimiao wrote:
The runtime-spec doesn't say process in container
should have such default capabilities, so remove them.
‘generate’ is not about spec compliance (that's ‘validate’), it's
about creating a useful starting point for users. So runtime-tools
maintainers are free to do whatever they like in ‘New()’ if they think
it makes life easier for folks who don't have existing opinions about
what they want in the spec.
|
On 05/11/2017 01:19 PM, W. Trevor King wrote:
‘generate’ is not about spec compliance (that's ‘validate’), it's
about creating a useful starting point for users.
Agree.
But I think we can't make sure which capability is necessary for user,
just leave it alone without default value will be better.
|
On Wed, May 10, 2017 at 10:53:14PM -0700, Ma Shimiao wrote:
But I think we can't make sure which capability is necessary for
user, just leave it alone without default value will be better.
I agree, but with that approach the best default config is probably
the minimal one for the platform (e.g. [1]) with the additional
recommended mounts [2]. The current default is really somewhere
between “useful for newcomers out of the box” and “demonstrates a
bunch of things so you can trim it down”. The latter goal is also
served by [3], so maybe this PR is the first stage of a pivot towards
a more minimal default?
[1]: https://github.com/opencontainers/runtime-spec/blob/1259a08e003658f8c4094a6bbc9f52aee29c95df/schema/test/config/good/minimal.json
[2]: https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#default-filesystems
[3]: https://github.com/opencontainers/runtime-spec/blob/master/config.md#configuration-schema-example
|
In my own opinion, |
On Thu, May 11, 2017 at 12:44:47AM -0700, Ma Shimiao wrote:
In my own opinion, `generate` should output config info based on
what runtime-spec minimally request in default.
So why is this PR just trimming the capabilities? If the goal is
minimal (including the SHOULD-level mounts [1]?), you could work up a
much bigger cut pretty quickly. Are there future plans for that and
some reason that taking this one group of properties at a time?
[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/config-linux.md#default-filesystems
|
Before I sent this PR, I haven't think so much about minimal or not. I just found the default caps will cause runtimetest fails, so began to fix it. |
We can use templates for minimal. |
0b6fe92
to
02a77b3
Compare
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.
Also “minial” → “minimal” in the commit message summary and the PR topic.
generate/generate.go
Outdated
@@ -39,180 +39,17 @@ func New() Generator { | |||
OS: runtime.GOOS, | |||
Arch: runtime.GOARCH, | |||
}, | |||
Root: rspec.Root{ | |||
Path: "", |
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.
This is a REQUIRED property, so I think you should keep this. If you don't like the empty-string value, I'd be ok with a "FIXME"
default.
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.
Because option --rootfs-path has default value rootfs
, then I think keep it or not doesn't matter , so removed it.
generate/generate.go
Outdated
User: rspec.User{}, | ||
User: rspec.User{ | ||
UID: 0, | ||
GID: 0, |
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.
These are the Go zero values, so you can stick with User: rspec.User{}
. I'm not opposed to explicitly setting them (like you're currently doing in this PR) though.
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.
Fine.
generate/generate.go
Outdated
Hostname: "mrsdalloway", | ||
Mounts: []rspec.Mount{ | ||
{ | ||
Destination: "/proc", |
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.
I think you want to keep this (and a few of the other entries) to follow the runtime's SHOULD.
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.
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.
Should we let spec should-level properties in minimal template?
I think so, because there is no reason to assume all callers have "carefully weighed (the full implications) before choosing a different course". Callers who have can always clear mounts
.
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.
OK, updated
02a77b3
to
35506cf
Compare
35506cf
to
5bea74e
Compare
@wking @mrunalp @hqhq @liangchenye PTAL |
With this title 'output minial tempalte in default', I prefer to provide template file for 'linux/windows/solaris' directly, just like: Maybe we can add them to |
Agree, we can supply default good templates. But I think without users' spcified options, generate outputs minimal template is also right. |
in default generate ouput which contains minial requests of spec Signed-off-by: Ma Shimiao <[email protected]>
5bea74e
to
da5029b
Compare
The runtime-spec doesn't say process in container
should have such default capabilities, so remove them.
Signed-off-by: Ma Shimiao [email protected]