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.

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 runtime-validate command to oci-runtime-tool #326

Merged
merged 4 commits into from
Mar 7, 2017

Conversation

mrunalp
Copy link
Contributor

@mrunalp mrunalp commented Feb 17, 2017

This is a conversion of the script to golang. There are a couple of TODOs which will be implemented in follow on PRs.
The intent here is to move away from the script to go and then start building on top of that.

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@mrunalp mrunalp changed the title Add runtime-validation command to oci-runtime-tool Add runtime-validate command to oci-runtime-tool Feb 17, 2017
func runtimeValidate(runtime string) error {
fmt.Println("-----------------------------------------------------------------------------------")
fmt.Printf(" VALIDATING RUNTIME: %q\n", runtime)
fmt.Println("-----------------------------------------------------------------------------------")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this header is really necessary ;). Certainly not without a way to turn it off. But if removing it is part of post-PR polishing, then whatever, we can keep it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

}

// Copy the runtimetest binary to the rootfs
err = fileutils.CopyFile("runtimetest", filepath.Join(bundleDir, "/runtimetest"))
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably don't need/want the leading slash (that's what filepath.Join is for).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@wking
Copy link
Contributor

wking commented Feb 18, 2017

I'm not sure how much you intend to handle with this PR, but you may want a Makefile rule to compile this. And if the goal is to make it installable, it will have to compile-in (or install?) a copy of runtimetest and not rely on its being in the current working directory.

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@mrunalp
Copy link
Contributor Author

mrunalp commented Feb 18, 2017

@wking Makefile already compiles oci-runtime-tool. This PR adds a new sub-command to it.
We could install the runtimetest, but given that it's a test binary, I am not too excited about that option. We could just make it an option to the command defaulting to looking in the current working directory.

@wking
Copy link
Contributor

wking commented Feb 18, 2017 via email

@Mashimiao
Copy link

Though --runtimetest option is not the best way, I think we can fix it later. I hope this can be landed as soon as possible.

@liangchenye
Copy link
Member

I agree with this. We can merge this and implement the remain 'TODO' later.
'runtime-tool' is more like a framework. It is hard to be done in one PR.

@Mashimiao @hqhq

@liangchenye
Copy link
Member

liangchenye commented Mar 6, 2017

LGTM

Approved with PullApprove

1 similar comment
@Mashimiao
Copy link

Mashimiao commented Mar 6, 2017

LGTM

Approved with PullApprove

@mrunalp mrunalp mentioned this pull request Mar 6, 2017
@hqhq
Copy link
Contributor

hqhq commented Mar 7, 2017

LGTM

Approved with PullApprove

@hqhq hqhq merged commit c1347ab into opencontainers:master Mar 7, 2017
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.

5 participants