Skip to content

Conversation

@liangchenye
Copy link
Member

Signed-off-by: liangchenye [email protected]

@wking
Copy link
Contributor

wking commented Feb 23, 2016

4ed52d9 looks good to me, although I'd recommend documenting this in
the Markdown spec as well (e.g. here 1). This is somewhat
complicated by the fact that our existing Markdown specs only define
the value of ‘linux’ entries, and don't specify the keys (e.g. what's
inside ‘linux.namespaces’, but not ‘namespaces’ itself 2). I think
we want an entry here 3 that talks about the possible ‘linux’
children and links to their more detailed docs. Something like:

where you could list apparmorProfile, selinuxProcessLabel, and
seccomp.

@crosbymichael
Copy link
Member

Can you explain why you want to do this?

config_linux.go Outdated
Devices []Device `json:"devices"`
// ApparmorProfile specified the apparmor profile for the container.
ApparmorProfile string `json:"apparmorProfile"`
ApparmorProfile *string `json:"apparmorProfile,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

These strings do not need to be pointers, only an omitempty

@liangchenye
Copy link
Member Author

@crosbymichael They are optional although are not mentioned in the spec. Most other fields has 'omitempty', I think they should be consistent. I'll update both config_linux.go and .md file and remove pointer.

@liangchenye
Copy link
Member Author

@wking I changed the original description by adding (type, optional) to the field name.

-SELinux process label specifies the label ...
+selinuxProcessLabel (string, optional) specifies the label...

@wking
Copy link
Contributor

wking commented Feb 24, 2016

On Tue, Feb 23, 2016 at 10:14:16PM -0800, 梁辰晔 (Liang Chenye) wrote:

@wking I changed the original description by adding (type, optional) to the field name.

-SELinux process label specifies the label ...
+selinuxProcessLabel (string, optional) specifies the label...

The problem with documenting that in the SELinux section is that it's
not clear where the entry lives in the config. By documenting the
entry in the parent section (e.g. the section describing ‘linux’ for
‘selinuxProcessLabel’, as I sketched out in 1), it becomes really
obvious that ‘selinuxProcessLabel’ is a direct child of ‘linux’.
Having a list of ‘linux’ children would also serve as a convenient
table of contents for this page.

@crosbymichael
Copy link
Member

These have been moved to the process and I have changed them to optional

@wking
Copy link
Contributor

wking commented Mar 2, 2016

On Wed, Mar 02, 2016 at 01:03:25PM -0800, Michael Crosby wrote:

These have been moved to the process and I have changed them to
optional

Cross-linking #329, which also addresses my “we should document the
optionality in the parent's section” concern 1 by shifting all the
docs into the process section 2.

@wking
Copy link
Contributor

wking commented Mar 2, 2016

On Wed, Mar 02, 2016 at 01:03:25PM -0800, Michael Crosby wrote:

These have been moved to the process and I have changed them to optional

Actually, it looks like seccomp (made an omitempty pointer in this
commit) was not moved to the process section in #329. Maybe reopen
this PR and rebase around the landed #329 to land the optional
seccomp?

@duglin
Copy link
Contributor

duglin commented Mar 2, 2016

Here's a link to the docs from the f2f where we decided which fields were optional vs required:
master...philips:hack-day-changes

@wking
Copy link
Contributor

wking commented Mar 2, 2016

On Wed, Mar 02, 2016 at 02:39:52PM -0800, Doug Davis wrote:

Here's a link to the docs from the f2f where we decided which fields were optional vs required:
master...philips:hack-day-changes

That agrees that seccomp should be optional 1. And there is a
post-merge note in #329 about maybe moving seccomp over to the process
section as well 2.

This was referenced Mar 8, 2016
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.

4 participants