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

Jump to bottom

oci-image-tool: initial cut #65

Merged
merged 1 commit into from
May 19, 2016
Merged

oci-image-tool: initial cut #65

merged 1 commit into from
May 19, 2016

Conversation

s-urbaniak
Copy link
Collaborator

@s-urbaniak s-urbaniak commented May 4, 2016 •
edited
Loading

This is work in progress for the oci-image-tool, current TODOs:

@philips philips added this to the v0.2.0 milestone May 5, 2016
@s-urbaniak
Copy link
Collaborator Author

gojsonschema: upstream PR xeipuuv/gojsonschema#101

@s-urbaniak s-urbaniak force-pushed the validation branch 3 times, most recently from 6cf4824 to 59fddf8 Compare May 6, 2016 15:59
philips
philips reviewed May 6, 2016
View reviewed changes
Makefile
@@ -62,9 +62,18 @@ oci-validate-json: validate.go
oci-validate-examples: cmd/oci-validate-examples/main.go
go build ./cmd/oci-validate-examples
Copy link
Contributor

Choose a reason for hiding this comment

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

we can replace this once you get validation done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ack

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really. This tool extracts the examples from the spec for validation. Do we intend to replace the markdown extraction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am thinking of moving the markdown extraction to a markdown_test.go invocable via go test and not have a separate binary.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great. I am not sure why I didn't do that initially.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@stevvooe moved oci-validate-examples as a unit test, PTAL 71e5942. I did not do big refactorings, just a minimalistic 1:1 transition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also removed oci-validate-json since it is being replaced by oci-image-tool

@s-urbaniak s-urbaniak force-pushed the validation branch 8 times, most recently from 93c5e52 to 8cfb5d5 Compare May 8, 2016 15:22
@s-urbaniak
Copy link
Collaborator Author

@stevvooe can it be you mentioned to have manifest examples I can validate against? It'd be great to get those.

@stevvooe
Copy link
Contributor

@s-urbaniak They are extracted directly from the specification by oci-examples-validate. We can have that tool extract them and pipe them to another tool that can validate the examples, but that tool links the specification against the schema definitions.

stevvooe
stevvooe reviewed May 10, 2016
View reviewed changes
cmd/oci-image-tool/validate.go

// withFile opens the named file for reading and executes the given function fn if no error occured.
// If the given function returns an error it will be returned.
func withFile(name string, fn func(f io.Reader) error) error {
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 an odd pattern for Go. Just open the file in validatePath. withFile isn't used anywhere else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, removed it, we're validating files right now only anyways.

@s-urbaniak s-urbaniak force-pushed the validation branch 2 times, most recently from 471b4a8 to e768f02 Compare May 11, 2016 17:37
@s-urbaniak
Copy link
Collaborator Author

@stevvooe OK, I was assuming you have more samples, nevermind. As mentioned above I'll try to migrate the functionality of oci-examples-validate to a Go test function. The manifest markdown links against the media type so this should be doable, right?

@stevvooe
Copy link
Contributor

OK, I was assuming you have more samples, nevermind.

Sorry about that. I think the idea is that we would add these variations to specification to introduce them in context.

The manifest markdown links against the media type so this should be doable, right?

Yep. I think you can port this almost directly. Most of oci-examples-validate should be pulled into the presented package.

@s-urbaniak s-urbaniak force-pushed the validation branch 7 times, most recently from 4eeb12c to d287023 Compare May 12, 2016 15:38
stevvooe
stevvooe reviewed May 12, 2016
View reviewed changes
schema/spec_test.go
func main() {
flag.Parse()
func TestSpecExamples(t *testing.T) {
m, err := os.Open("../manifest.md")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's throw a TODO here to have this encompass all specification files.

@liangchenye
Copy link
Member

liangchenye commented May 13, 2016 •
edited
Loading

CooI! I think it might to have this feature in /~https://github.com/opencontainers/ocitools,
especially for the remain tasks:

 Validate unpacked image layout on disk
 Validate compressed image
 Introduce vendored dependencies
 rethink build system

What do you think? @s-urbaniak @mrunalp @philips

stevvooe
stevvooe reviewed May 13, 2016
View reviewed changes
schema/media_types.go

// ManifestValidator wraps a media type string identifier
// and implements validation against a JSON schema.
type ManifestValidator string
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit: these won't always be "manifests".

@philips
Copy link
Contributor

philips commented May 19, 2016

@liangchenye right, @s-urbaniak please don't worry about validating the ondisk format. We can rely on the oci-runtime-tool for that.

@philips
Copy link
Contributor

philips commented May 19, 2016

@s-urbaniak can we merge the tool as-is and do a follow-up PR? Essentially just add the dependencies and then we can do followup PRs for each of the remaining featrues?

@s-urbaniak
Copy link
Collaborator Author

@philips sure, we can mere as is! Let me squash all of this in a sec. I working on an unpacker now. What do you mean with "Essentially just add the dependencies"? Do you mean vendoring the deps?

@s-urbaniak
oci-image-tool: initial commit
4263cf5 
This adds the oci-image-tool with the following capabilities:
- validate manifest
- validate manifest list

Signed-off-by: Sergiusz Urbaniak <sur@coreos.com>
@s-urbaniak
Copy link
Collaborator Author

@philips squashed, ready to merge once green

@jonboulle jonboulle changed the title [WIP] oci-image-tool DO NOT MERGE oci-image-tool: initial cut May 19, 2016
@jonboulle jonboulle merged commit 6b5d048 into opencontainers:master May 19, 2016
@philips
Copy link
Contributor

philips commented May 19, 2016

LGTM, can you file individual issues for the remaining things?

@s-urbaniak
Copy link
Collaborator Author

@philips filed #74, #75, and #76 to catch remaining TODOs and updated the description.

@stevvooe
Copy link
Contributor

LGTM

I have no problems with merging this PR, but let's make sure to get a few more LGTMs in the future.

@vbatts
Copy link
Member

vbatts commented May 19, 2016

This was a bigger kick off of work, but so far we've been merging with one,
sometimes two LGTMs.

On Thu, May 19, 2016, 17:30 Stephen Day notifications@github.com wrote:

LGTM

I have no problems with merging this PR, but let's make sure to get a few
more LGTMs in the future.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#65 (comment)

@wking wking mentioned this pull request Feb 16, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
component/oci-image-tool
Projects
None yet
Milestone
v0.2.0
Development

Successfully merging this pull request may close these issues.
6 participants
@s-urbaniak @stevvooe @liangchenye @philips @vbatts @jonboulle