generate: Respect --os when generating default templates #270
Conversation
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
This is a bit aggressive. For example, maybe a Linux kernel was compiled with CONFIG_64BIT, CONFIG_X86, and CONFIG_X86_X32. Your ocitools build may be 386 and validating an amd64 image (or vice versa) and both would run fine. I'm not sure x32 is supported by Go, but you could have and x32 container if somebody submits it to the OCI specs as an extention arch. So I'd rather have an arch mismatch be a warning than a fatal error. But in most cases, you'll want the arch to match (e.g. no arm configs if ocitools is amd64), so I don't want to ignore the missmatch completely. In the absence of warning-support, an overly strict check seems safer than an overly lax check, because the caller can always read the error messages and decide if they care ;). Signed-off-by: W. Trevor King <[email protected]>
Don't fill in a bunch of Linux stuff if runtime.GOOS isn't Linux ;). We don't have sensible defaults for other OSes yet, so error out in those cases. Signed-off-by: W. Trevor King <[email protected]>
Also add --host-specific checks to SetPlatformOS and SetPlatformArch. The SetPlatformArch check is currently just a warning (details in eda1ac7, validate: With --host-specific, compare config platform vs. runtime, 2016-08-16) but I've added an error to its API so callers will be ready when we do actually raise errors (e.g. for '--arch arm' on an amd64 box). Also add IsSet checks to --os and --arch so we don't clobber their template values. Before this commit on my amd64 Linux box: $ echo '{"platform": {"os": "windows", "arch": "386"}}' >template.json $ ocitools generate --template template.json | jq .platform { "os": "linux", "arch": "amd64" } After this commit: $ ocitools generate --template template.json | jq .platform { "os": "windows", "arch": "386" } Signed-off-by: W. Trevor King <[email protected]>
wking
force-pushed
the
validate-host-platform-2
branch
from
1ae6388 to
fedbf90
Compare
mrunalp
reviewed
Nov 15, 2016
| // OS (which defaults to runtime.GOOS). | ||
| func New(os *string) (generator Generator, err error) { | ||
| var goos string | ||
| goos = runtime.GOOS |
Contributor
|
make failed in the build. |
mrunalp
reviewed
Nov 15, 2016
| Action: func(context *cli.Context) error { | ||
| // Start from the default template. | ||
| specgen := generate.New() | ||
| var osPointer *string |
Contributor
There was a problem hiding this comment.
Maybe rename the variable to specOs? I don't like that we have Pointer in the name. Go doesn't recommend hungarian notation.
Contributor
Author
Member
|
Since platform is removed, close this PR. |
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.
Also add
--host-specificchecks toSetPlatformOSandSetPlatformArch. TheSetPlatformArchcheck is currently just a warning (details in eda1ac7, validate: With --host-specific, compare config platform vs. runtime, 2016-08-16) but I've added an error to its API so callers will be ready when we do actually raise errors (e.g. for--arch armon an amd64 box).Also add IsSet checks to
--osand--archso we don't clobber their template values. Before this commit on my amd64 Linux box:After this commit:
This commit was previously part of #194, but I've pulled it out into its own PR because @Mashimiao isn't sold yet ;).