validate: Test config.json and rootfs sibling-hood#353
Merged
liangchenye merged 1 commit intoopencontainers:masterfrom Mar 30, 2017
Merged
validate: Test config.json and rootfs sibling-hood#353liangchenye merged 1 commit intoopencontainers:masterfrom
liangchenye merged 1 commit intoopencontainers:masterfrom
Conversation
Mashimiao
reviewed
Mar 29, 2017
validate/validate.go
Outdated
| } | ||
|
|
||
| rootParent := filepath.Dir(absRootPath); | ||
| if rootParent == string(filepath.Separator) || rootParent != absBundlePath { |
There was a problem hiding this comment.
You mean rootParent can't be "/"? It seems there is no such limitation in SPEC.
Contributor
Author
There was a problem hiding this comment.
You mean rootParent can't be "/"?
That should be fine. The Separator comparison is intended to catch absRootPath being a spelling of /, based on the Dir docs:
The returned path does not end in a separator unless it is the root directory.
But I'll double check Dir("/") later tonight.
Contributor
Author
wking
force-pushed
the
root-path-config-sibling
branch
from
078bb17 to
8d697e8
Compare
Mashimiao
reviewed
Mar 29, 2017
validate/validate.go
Outdated
| if err != nil { | ||
| msgs = append(msgs, fmt.Sprintf("unable to convert %q to an absolute path", v.bundlePath)) | ||
| } | ||
| absBundlePath = filepath.Clean(absBundlePath) |
There was a problem hiding this comment.
Maybe we don't need this, Abs() calls Clean() on the result.
Contributor
Author
validate/validate.go
Outdated
| var absRootPath string | ||
| if filepath.IsAbs(v.spec.Root.Path) { | ||
| rootfsPath = v.spec.Root.Path | ||
| absRootPath = rootfsPath |
There was a problem hiding this comment.
I think adding filepath.Clean() here will be fine
validate/validate.go
Outdated
| msgs = append(msgs, fmt.Sprintf("unable to convert %q to an absolute path", rootfsPath)) | ||
| } | ||
| } | ||
| absRootPath = filepath.Clean(absRootPath) |
There was a problem hiding this comment.
removing this from here will be fine.
Test the spec's [1]: While these artifacts MUST all be present in a single directory on the local filesystem, ... which is a condition it imposes on config.json and the directory referenced by root.path. I think we should drop that restriction from the spec, but my attempt to remove the restriction was rejected [2]. If a future spec drops the restriction, we can revert this commit. Using path/filepath for the path manipulation will break when validating cross-platform configs (e.g. trying to validate a Windows bundle on a Linux machine). But that's a bigger issue than this commit, so I've left it alone for now. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/bundle.md#container-format [2]: opencontainers/runtime-spec#469 Signed-off-by: W. Trevor King <wking@tremily.us>
wking
force-pushed
the
root-path-config-sibling
branch
from
8d697e8 to
899afea
Compare
|
LGTM |
1 similar comment
Member
|
LGTM |
Member
|
Interesting, CI fails in getting golint. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test the spec's:
which is a condition it imposes on
config.jsonand the directory referenced byroot.path.I think we should drop that restriction from the spec, but my attempt to remove the restriction was rejected. If a future spec drops the restriction, we can revert this commit.
Using
path/filepathfor the path manipulation will break when validating cross-platform configs (e.g. trying to validate a Windows bundle on a Linux machine). But that's a bigger issue than this commit (previous discussion here and here), so I've left it alone for now.