Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mashimiao
Copy link

The runtime-spec doesn't say process in container
should have such default capabilities, so remove them.

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

@wking
Copy link
Contributor

wking commented May 11, 2017 via email

@Mashimiao
Copy link
Author

Mashimiao commented May 11, 2017 via email

@wking
Copy link
Contributor

wking commented May 11, 2017 via email

@Mashimiao
Copy link
Author

In my own opinion, generate should output config info based on what runtime-spec minimally request in default. Any optional or additional fields in config should be specified by users which may be by options of command generate.

@wking
Copy link
Contributor

wking commented May 11, 2017 via email

@Mashimiao
Copy link
Author

Mashimiao commented May 11, 2017

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.

@mrunalp
Copy link
Contributor

mrunalp commented May 12, 2017

We can use templates for minimal.

@Mashimiao Mashimiao force-pushed the generate-remove-def-cap branch from 0b6fe92 to 02a77b3 Compare May 15, 2017 03:52
@Mashimiao Mashimiao changed the title generate: remove default capabilities generate: output minial template in default May 15, 2017
@Mashimiao
Copy link
Author

@wking @mrunalp PTAL

Copy link
Contributor

@wking wking left a 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.

@@ -39,180 +39,17 @@ func New() Generator {
OS: runtime.GOOS,
Arch: runtime.GOARCH,
},
Root: rspec.Root{
Path: "",
Copy link
Contributor

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.

Copy link
Author

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.

User: rspec.User{},
User: rspec.User{
UID: 0,
GID: 0,
Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

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

Fine.

Hostname: "mrsdalloway",
Mounts: []rspec.Mount{
{
Destination: "/proc",
Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

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

I think that's a question.
Should we let spec should-level properties in minimal template? @wking @mrunalp

Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

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

OK, updated

@Mashimiao Mashimiao force-pushed the generate-remove-def-cap branch from 02a77b3 to 35506cf Compare May 17, 2017 02:48
@Mashimiao Mashimiao changed the title generate: output minial template in default generate: output minimal template in default May 17, 2017
@Mashimiao Mashimiao force-pushed the generate-remove-def-cap branch from 35506cf to 5bea74e Compare May 23, 2017 06:43
@Mashimiao
Copy link
Author

@wking @mrunalp @hqhq @liangchenye PTAL

@liangchenye
Copy link
Member

With this title 'output minial tempalte in default', I prefer to provide template file for 'linux/windows/solaris' directly, just like:
https://github.com/opencontainers/runtime-spec/tree/master/schema/test/config/good

Maybe we can add them to runtime-tools/contrib/templates?

@Mashimiao
Copy link
Author

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 <mashimiao.fnst@cn.fujitsu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants