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.

Sign up for GitHub

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

Add LayerFolders to Windows platform config #828

Merged
merged 1 commit into from
May 30, 2017
Merged

Add LayerFolders to Windows platform config #828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions config-windows.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,22 @@
This document describes the schema for the [Windows-specific section](config.md#platform-specific-configuration) of the [container configuration](config.md).
The Windows container specification uses APIs provided by the Windows Host Compute Service (HCS) to fulfill the spec.

## <a name="configWindowsLayerFolders" />LayerFolders

**`layerFolders`** (array of strings, REQUIRED) specifies a list of layer folders the container image relies on. The list is ordered from topmost layer to base layer.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the first instance (as far as I know) of a REQUIRED property that is a direct child of a platform property (windows, in this case). Because REQUIRED is only “if the parent is set”, I think you'll want to adjust this section to say that windows MUST be set when platform.os is windows.

Discussion in #830 may end up with platform being removed entirely. If that lands first, it will change the phrasing for the windows requirement, but requiring the windows object for configs aimed at the windows platform will still be something you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated config.md to specify that the windows configuration section is required if the windows platform is set. We can update this again if #830 lands before this PR.

`layerFolders` MUST contain at least one entry.

### Example

```json
"windows": {
"layerFolders": [
"C:\\Layers\\layer1",
"C:\\Layers\\layer2"
]
}
```

## <a name="configWindowsResources" />Resources

You can configure a container's resource limits via the OPTIONAL `resources` field of the Windows configuration.
Expand Down
2 changes: 1 addition & 1 deletion config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ Runtime implementations MAY support any valid values for platform-specific field
* **`linux`** (object, OPTIONAL) [Linux-specific configuration](config-linux.md).
This MAY be set if **`platform.os`** is `linux` and MUST NOT be set otherwise.
* **`windows`** (object, OPTIONAL) [Windows-specific configuration](config-windows.md).
This MAY be set if **`platform.os`** is `windows` and MUST NOT be set otherwise.
This MUST be set if **`platform.os`** is `windows` and MUST NOT be set otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely clear on the maintainers' preference for this (@mrunalp was going to document them), but it's possible that they'll want *Windows changed to Windows in the Go structure now that this property is required on one platform. At least, something like that was the reason given for not wanting pointers for User.UID and User.GID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats an interesting point. Will it be a little odd that the other platform specific fields are not pointers as well? I would expect the maintainers would like to keep these fields uniform.

* **`solaris`** (object, OPTIONAL) [Solaris-specific configuration](config-solaris.md).
This MAY be set if **`platform.os`** is `solaris` and MUST NOT be set otherwise.

Expand Down
13 changes: 12 additions & 1 deletion schema/config-windows.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@
"id": "https://opencontainers.org/schema/bundle/windows",
"type": "object",
"properties": {
"layerFolders": {
"id": "https://opencontainers.org/schema/bundle/windows/layerFolders",
"type": "array",
"items": {
"$ref": "defs.json#/definitions/FilePath"
},
"minItems": 1
},
"resources": {
"id": "https://opencontainers.org/schema/bundle/windows/resources",
"type": "object",
Expand Down Expand Up @@ -66,6 +74,9 @@
}
}
}
}
},
"required": [
"layerFolders"
]
}
}
2 changes: 2 additions & 0 deletions specs-go/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,8 @@ type SolarisAnet struct {

// Windows defines the runtime configuration for Windows based containers, including Hyper-V containers.
type Windows struct {
// LayerFolders contains a list of absolute paths to directories containing image layers.
LayerFolders []string `json:"layerFolders"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Just spotted this. Should be all lower-case for consistency. And the config-windows.json schema file needs the matching update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be all lower-case for consistency.

There are currently some camelCase entries in config-windows.md. For example, the network properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

@darrenstahlmsft Could you open a PR to fix ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking @jhowardmsft i modified the field name as per this suggestion, should I revert it back?

It looks like there are linux fields that use camel case as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there are linux fields that use camel case as well.

The Linux config tries to consistently use camelCase. I don't know if the maintainers care what convention the Windows-only fields use; probably wait for them to weigh in.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, lets leave this as is. I'll take an action item to clean up the casing once this is merged. Opened #857 to track.

// Resources contains information for handling resource constraints for the container.
Resources *WindowsResources `json:"resources,omitempty"`
}
Expand Down