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

validate: add namespace if is duplicated check #295

Merged

Conversation

Mashimiao
Copy link

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

}

nsType := v.spec.Linux.Namespaces[index].Type
nsPath := v.spec.Linux.Namespaces[index].Path
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd rather have ns := v.spec.Linux.Namespaces[index]. Only one variable (vs. the two here), and you only need an extra character when you use it (e.g. ns.Type vs your nsType).

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

@wking
Copy link
Contributor

wking commented Dec 22, 2016

de21837 looks good to me, although I'd slightly prefer a single convenience variable.

Do we want to link here from the error message? Or are we punting on that until we're ready for some future push on #16?

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao Mashimiao force-pushed the validate-add-dup-namespace-check branch from de21837 to eea002f Compare December 23, 2016 01:11
@Mashimiao
Copy link
Author

@wking I think we'd better don't link from error message in this PR. If we want to add links, we should find out all error message related with spec. part of messages have link, part of not, that does not look unified. And maybe adding links will cause maintaining problem, that needs more discussion

@Mashimiao
Copy link
Author

@hqhq @mrunalp @liangchenye PTAL

@Mashimiao
Copy link
Author

ping @opencontainers/runtime-tools-maintainers

@liangchenye
Copy link
Member

liangchenye commented Mar 21, 2017

LGTM

Approved with PullApprove

1 similar comment
@mrunalp
Copy link
Contributor

mrunalp commented Mar 24, 2017

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 21af41d into opencontainers:master Mar 24, 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.

4 participants