bundle: DRYer root.path entry #840
Conversation
|
Please rebase |
Rebased around #846 with 22836ee → b420c96. The Travis error looks like a network hiccup. If you want that green you can probably just kick the build again. |
bundle.md
Outdated
| On all other platforms, this field is REQUIRED. | ||
|
|
||
| If set, this directory MUST be referenced by [`root`](config.md#root) within the `config.json` file. | ||
| 2. <a name="containerFormat02" />The directory referenced by [`root.path`](config.md#root), if that property is set in `config.json`. |
There was a problem hiding this comment.
A directory referenced by ... will be better?
There was a problem hiding this comment.
A directory referenced by ...will be better?
There is only one directory referenced by a given root.path value, so I prefer the definite article. But if you can get a second opinion in favor of the indefinite "A", I'll change it.
There was a problem hiding this comment.
I think there is doubt, directory is the subject of this sentence. Here, we want to express there is a directory which is referenced by config.md#root. The directory will stress that directory is a specific directory, that's OK, but the base is everyone know which and what the directory is. I think there is not such base in context, it looks unreasonable.
If I'm wrong, please feel free to correct me.
There was a problem hiding this comment.
Here, we want to express there is a directory which is referenced by config.md#root.
Yes, and it is that directory which is the artifact.
The directorywill stress that directory is a specific directory, that's OK, but the base is everyone know which and what the directory is. I think there is not such base in context, it looks unreasonable.
But everyone should know which directory it is, since we say "referenced by root.path, if that property is set in config.json". So the previous entry establishes the config.jsonartifact, and there can be only one file at that path. And this entry establishes (when root.path is set in that configuration) the referenced directory as an artifact too. Once you have the config.json, there is no further choice about which directory you use for this artifact.
There was a problem hiding this comment.
OK, How about as following:
2. container's root filesystem: the directory referenced by root.path, if that property is set in config.json.
keep same format with previous entry, just a suggestion.
There was a problem hiding this comment.
keep same format with previous entry...
Sure. I can push that up tomorrow.
wking
force-pushed
the
dry-bundle-hyper-v
branch
from
b420c96 to
dbfae14
Compare
Instead of repeating the config.md conditions, just make the linkage more clear. Now the chain is: 1. Bundles REQUIRE a config.json which is a bundle artifact. 2. If that config.json has a root.path entry (as specified in config.md), then add the referenced directory to the set of bundle artifacts. The config.md requirements include "If defined, a directory MUST exist at the path declared by the field". 3. Apply the "MUST all be present in a single directory" condition to all bundle artifacts. I don't like that direct-child restriction [1], but I'm not touching it in this commit. So these are the same requirements as before this commit, but with less redundancy and fewer words. [1]: opencontainers#469 Signed-off-by: W. Trevor King <[email protected]>
|
LGTM |
1 similar comment
|
LGTM |
Instead of repeating the config.md conditions, just make the linkage more clear. Now the chain is:
config.jsonwhich is a bundle artifact.config.jsonhas aroot.pathentry (as specified inconfig.md), then add the referenced directory to the set of bundle artifacts. Theconfig.mdrequirements include “If defined, a directory MUST exist at the path declared by the field”.So these are the same requirements as before this commit, but with less redundancy and fewer words.