Skip to content

runtimetest: add host platform validation#507

Merged
Mashimiao merged 1 commit intoopencontainers:masterfrom
zhouhao3:validate-platform-runtimetest
Nov 17, 2017
Merged

runtimetest: add host platform validation#507
Mashimiao merged 1 commit intoopencontainers:masterfrom
zhouhao3:validate-platform-runtimetest

Conversation

@zhouhao3
Copy link
Copy Markdown

@zhouhao3 zhouhao3 commented Nov 3, 2017

Signed-off-by: Zhou Hao zhouhao@cn.fujitsu.com


func validatePlatform(spec *rspec.Spec) error {
if runtime.GOOS != "linux" && runtime.GOOS != "solaris" && runtime.GOOS != "windows" {
return fmt.Errorf("host platform %q is not supported", runtime.GOOS)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is presumably based on this spec section, but there's no RFC 2119 language there, and the platform target is closer to selecting the relevant spec than it is to being a requirement of the spec. I'm fine with having something like this being a fatal error (i.e. “runtime-tools has not implemented testing for your platform because the spec has nothing to say about it”). But I don't see a point to making it soft error after which we continue on to run our other tests. If we're comfortable running the other tests, we should just run them, and let each test take care of skipping all the things that are specific to one of the above platforms (which is most things).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated, PTAL.

@zhouhao3 zhouhao3 force-pushed the validate-platform-runtimetest branch from c557bac to 7f1a267 Compare November 3, 2017 05:36
}

platform := runtime.GOOS
if err := validatePlatform(platform); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be moved before loadSpecConfig. Non need to hit the disk at all for unsupported platforms.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated. PTAL.

@zhouhao3 zhouhao3 force-pushed the validate-platform-runtimetest branch from 7f1a267 to 1f59e78 Compare November 6, 2017 01:44

func validatePlatform(platform string) error {
if platform != "linux" && platform != "solaris" && platform != "windows" {
return fmt.Errorf("runtime-tools has not implemented testing for your platform %q, because the spec has nothing to say about it", platform)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is hard to say whether right or not.
Though spec don't have specific configs for those platform, But spec has common configs for all platform.
And spec seems never say it just supports those three platforms.

@Mashimiao Mashimiao added this to the v0.4.0 milestone Nov 15, 2017
@liangchenye
Copy link
Copy Markdown
Member

I think this judgement is useful, testing on an unknown platform can not prove anything, either right or wrong. We can just abort the testing.

I think we should not name it 'validatePlatform', it is misleading. It sounds like there is a RFC requirement in the spec and we are validating it. But in fact, we are just making sure runtime-tool works as expected.

I suggest we can just do it in the run function.

func run(context *cli.Context) error {
        ....

	platform := runtime.GOOS	
        if platform != "linux" && platform != "solaris" && platform != "windows" {
		return fmt.Errorf("runtime-tools has not implemented testing for your platform %q, because the spec has nothing to say about it", platform)
	}
}

Signed-off-by: Zhou Hao <zhouhao@cn.fujitsu.com>
@zhouhao3 zhouhao3 force-pushed the validate-platform-runtimetest branch from 1f59e78 to 39f3f74 Compare November 17, 2017 08:30
@zhouhao3
Copy link
Copy Markdown
Author

updated, thanks.

@liangchenye
Copy link
Copy Markdown
Member

liangchenye commented Nov 17, 2017

LGTM

Approved with PullApprove

1 similar comment
@Mashimiao
Copy link
Copy Markdown

Mashimiao commented Nov 17, 2017

LGTM

Approved with PullApprove

@Mashimiao Mashimiao merged commit 8a6d284 into opencontainers:master Nov 17, 2017
@zhouhao3 zhouhao3 deleted the validate-platform-runtimetest branch November 17, 2017 08:34
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