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

remove comment #714

Merged
merged 1 commit into from
Mar 10, 2017
Merged

Conversation

zhouhao3
Copy link

@zhouhao3 zhouhao3 commented Mar 7, 2017

Signed-off-by: zhouhao [email protected]

@cyphar
Copy link
Member

cyphar commented Mar 7, 2017

NACK. There's a very good reason that JSON doesn't support comments. While you might not agree with it, it is a valid reason. Not to mention that the runtime-spec specifically tells runtimes to ignore fields they don't recognise (so using comment should already work).

@crosbymichael
Copy link
Member

What does this PR actually do? I don't understand.

@wking
Copy link
Contributor

wking commented Mar 8, 2017 via email

@cyphar
Copy link
Member

cyphar commented Mar 8, 2017

-_- Yeah, IMO that should be reverted -- what was the original reason for that field?

@crosbymichael
Copy link
Member

I think that just slipped by. Its not necessary for the spec so we should probably remove it. The spec should be short and concise, only containing information required for the creation/execution of a container.

I can see that a comment is a nice to have but not required and probably shouldn't be in the spec.

@zhouhao3
Copy link
Author

zhouhao3 commented Mar 8, 2017

So the result of your discussion is to delete the comment field? Is there a clear rule to delete comment?

@crosbymichael
Copy link
Member

@q384566678 ya, I don't think it should have been added in the first place and just slipped by in review. We don't have any other comment fields in the spec.

@zhouhao3
Copy link
Author

zhouhao3 commented Mar 9, 2017

@crosbymichael As you said, I think should delete the comment in the spec, as I did in b7a172d.

@zhouhao3 zhouhao3 changed the title defs-linux.json: add comment remove comment Mar 9, 2017
@cyphar
Copy link
Member

cyphar commented Mar 9, 2017

👍

config-linux.md Outdated
@@ -549,8 +549,7 @@ Operator Constants:
"getcwd",
"chmod"
],
"action": "SCMP_ACT_ERRNO",
"comment": "stop exploit x"
"action": "SCMP_ACT_ERRN"
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

Copy link
Author

Choose a reason for hiding this comment

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

updated, PTAL

Signed-off-by: zhouhao <[email protected]>
@hqhq
Copy link
Contributor

hqhq commented Mar 10, 2017

LGTM

Approved with PullApprove

1 similar comment
@crosbymichael
Copy link
Member

crosbymichael commented Mar 10, 2017

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 6bfef10 into opencontainers:master Mar 10, 2017
@zhouhao3 zhouhao3 deleted the seccomp-commits branch March 22, 2017 06:48
@vbatts vbatts mentioned this pull request Jul 5, 2017
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