tools/refenginediscovery: Refactor Ref-Engine Discovery library API #29

wking · 2017-09-20T20:02:42Z

Pull the UI portions of this out into tools/cmd, because the library API should not depend on urfave/cli.

The new refenginediscovery API is based on a per-configured-ref-engine callback, which gives callers lots of flexibility in how they want to handle each ref-engine and if/when they want to abort the protocol/host iteration.

I've also adjusted the ref-engine interface to return a new MerkleRoot type. This improves on the old Descriptor returns by:

Being less opinionated about the root object, to stay in keeping with ref-engine-discovery.md's:

The Merkle root may be a descriptor (media type application/vnd.oci.descriptor.v1+json), or it may have a different media type.
Including the retrieval URI, which may be needed to expand references in the root object (e.g. if the root has a casEngines configuration which includes a relative reference).

These changes bring the API and output of the oci-discovery resolve … command much closer to the current Python module. The remaining differences are the presence of a roots indirection in the Python output (which I'm planning on removing) and the lack of uri entries for each root (which I'm planning on adding).

wking · 2017-09-20T20:09:48Z

The remaining differences are the presence of a roots indirection in the Python output (which I'm planning on removing)…

Actually, we may want to keep this. The Python implementation currently uses to pass through ref-engines-object-level casEngines. We may want to follow that lead in Go. There are alternatives, like injecting those casEngines entries into the MerkleRoot.Root object, but that gets tricky now that it's an interface{}. We can also add a new MerkleRoot.CASEngines and store the ref-engines-object-level entries (and any others we collect from other places) there.

wking · 2017-09-20T20:15:17Z

The Python UI is growing the uri entries in #25.

wking · 2017-09-20T20:23:14Z

tools/refengine/root.go

+
+	// The Merkle root object.  While this may be of any type.  OCI
+	// tools will generally use image-spec Descriptors.
+	Root interface{}


We may want a MediaType string property on MerkleRoot so we don't lose typing information in any JSON output. We still have Go typing information without it, because callers can use reflection and/or casting to determine if Root is backed by a type they understand. But having an explicit media type stores that information in a way that will survive JSON serialization. I can add that to this commit, or make it part of follow-up work.

We may want a MediaType string property…

Added with 9529456 → bb96e49.

wking · 2017-09-21T05:50:30Z

Rebased around some recent changes with dc87b41 → 76a7635. I'm still not sure what the best way is to make this go get …able after #27, but at least with these changes we can compile the executable again ;).

wking · 2017-09-21T06:13:06Z

I think the vendor thing is grounds to create separe spec, lib, and CLI-tool repos. I'll work that up tomorrow.

xiekeyang · 2017-09-21T06:52:51Z

Sorry I am cognizant that #27 is a careless commit, which leads code structure some confusion. So I have reverted it #31 . We should figure out a better way later.

xiekeyang · 2017-09-21T06:54:07Z

I would help to resolve the conflicts now.

xiekeyang · 2017-09-21T07:49:45Z

I split commit f57ae08 to #33.

The order is (expected, actual, ...) [1]. Reported by Keyang in [2]. [1]: https://godoc.org/github.com/stretchr/testify/assert#Equal [2]: xiekeyang@9139cfd#diff-79f54ccc4674abcf6ef6ae61d5dba64bL322

The old strings are what Go 1.7.4 generated, but Keyang reports another Go version generating more specific strings [1]. [1]: xiekeyang@9139cfd#diff-79f54ccc4674abcf6ef6ae61d5dba64bL298

wking · 2017-09-21T16:45:44Z

Rebased around #31 with 881db73 → 9529456. With #31, we can continue to punt on splitting this out into separate repos.

Pull the UI portions of this out into tools/cmd, because the library API should not depend on urfave/cli. The new refenginediscovery API is based on a per-configured-ref-engine callback, which gives callers lots of flexibility in how they want to handle each ref-engine and if/when they want to abort the protocol/host iteration. I've also adjusted the ref-engine interface to return a new MerkleRoot type. This improves on the old Descriptor returns by: * Being less opinionated about the root object, to stay in keeping with ref-engine-discovery.md's: The Merkle root may be a descriptor (media type application/vnd.oci.descriptor.v1+json), or it may have a different media type. The new MediaType parameter provides guidance for tools that need to consume the otherwise-opaque Root data. Go tools can use reflection and/or casting to determine if Root is a type they support, but the mediaType JSON entry will preserve that information through JSON serialization. It's also as convenient as reflection without requiring a 'reflect' import, and it's much more convenient than iterating through casts to known types. * Including the retrieval URI, which may be needed to expand references in the root object (e.g. if the root has a casEngines configuration which includes a relative reference). These changes bring the API and output of the 'oci-discovery resolve ...' command much closer to the current Python module. The remaining differences are the presence of a 'roots' indirection in the Python output (currently used to pass through ref-engines-object-level casEngines entries) and the lack of 'uri' entries for each root (which I'm planning on adding).

xiekeyang · 2017-09-22T01:36:26Z

LGTM

Previously these were not provided in the output JSON. To store the data during resolution, create a new resolvedName type. That type extends the usual MerkleRoot JSON with a new 'casEngines' property that is a sibling of MerkleRoot's 'root' and 'uri' properties. To get the sibling serialization, we need a little MarshalJSON wrapper for resolvedName, but we don't need to bother with UnmarshalJSON because this isn't a public library type. Rename 'allRoots' to 'resolvedNames', because each key in the map is an image name. Each value in the map may contain several roots (resolveNames), so 'allRoots' wasn't particularly clear. Move refenginediscovery.ResolvedCASEngine to engine.Reference. This gives us a shorter name, and lets us recycle some code via the new (private) (*Config).unmarshalInterface. That lets us get the same config serialization in Reference without the wash through bytes that I use in resolvedName.MarshalJSON. Washing through bytes is an acceptable hack for command-line-specific code, but it's nice to avoid it in the library code. While we're at it, cleanup the MerkleRoot JSON handling, dropping the tag from MediaType (redundant because our JSON helpers don't use reflection) and adding comments for the public methods. Alphabetizing the root entries in the oci-discovery README example probably should have happened back in bb96e49 (tools/refenginediscovery: Refactor Ref-Engine Discovery library API, 2017-09-20, xiekeyang#29), since that's when we started using a map[string]interface{} in MerkleRoot.MarshalJSON.

Previously these were not provided in the output JSON. To store the data during resolution, create a new resolvedName type. That type extends the usual MerkleRoot JSON with a new 'casEngines' property that is a sibling of MerkleRoot's 'root' and 'uri' properties. To get the sibling serialization, we need a little MarshalJSON wrapper for resolvedName, but we don't need to bother with UnmarshalJSON because this isn't a public library type. Rename 'allRoots' to 'resolvedNames', because each key in the map is an image name. Each value in the map may contain several roots (resolveNames), so 'allRoots' wasn't particularly clear. Move refenginediscovery.ResolvedCASEngine to engine.Reference. This gives us a shorter name, and lets us recycle some code via the new (private) (*Config).unmarshalInterface. That lets us get the same config serialization in Reference without the wash through bytes that I use in resolvedName.MarshalJSON. Washing through bytes is an acceptable hack for command-line-specific code, but it's nice to avoid it in the library code. While we're at it, clean up the MerkleRoot JSON handling, dropping the tag from MediaType (redundant because our JSON helpers don't use reflection) and adding comments for the public methods. Alphabetizing the root entries in the oci-discovery README example probably should have happened back in bb96e49 (tools/refenginediscovery: Refactor Ref-Engine Discovery library API, 2017-09-20, xiekeyang#29), since that's when we started using a map[string]interface{} in MerkleRoot.MarshalJSON.

wking force-pushed the go-ref-engine-discovery-api branch from 726c1c7 to dc87b41 Compare September 20, 2017 20:06

wking commented Sep 20, 2017

View reviewed changes

wking force-pushed the go-ref-engine-discovery-api branch from dc87b41 to 76a7635 Compare September 21, 2017 05:48

xiekeyang closed this Sep 21, 2017

This was referenced Sep 21, 2017

tools/refenginediscovery: Refactor Ref-Engine Discovery library API #32

Closed

[Splited commit]: tools/refenginediscovery: Refactor Ref-Engine Discovery library API #33

Closed

xiekeyang reopened this Sep 21, 2017

wking added 2 commits September 21, 2017 09:32

tools/*/*_test: Flip assert.Equal order

3dc6bd8

The order is (expected, actual, ...) [1]. Reported by Keyang in [2]. [1]: https://godoc.org/github.com/stretchr/testify/assert#Equal [2]: xiekeyang@9139cfd#diff-79f54ccc4674abcf6ef6ae61d5dba64bL322

tools/indextemplate/indextemplate_test: Use regexps for parsing errors

bbf47ec

The old strings are what Go 1.7.4 generated, but Keyang reports another Go version generating more specific strings [1]. [1]: xiekeyang@9139cfd#diff-79f54ccc4674abcf6ef6ae61d5dba64bL298

wking force-pushed the go-ref-engine-discovery-api branch from 881db73 to 9529456 Compare September 21, 2017 16:44

wking force-pushed the go-ref-engine-discovery-api branch from 9529456 to bb96e49 Compare September 21, 2017 20:14

xiekeyang merged commit 6b9c9a5 into xiekeyang:master Sep 22, 2017

wking deleted the go-ref-engine-discovery-api branch September 22, 2017 21:33

wking mentioned this pull request Sep 23, 2017

tools: Expose ref-engine discovery casEngines #39

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

tools/refenginediscovery: Refactor Ref-Engine Discovery library API #29

tools/refenginediscovery: Refactor Ref-Engine Discovery library API #29

Uh oh!

wking commented Sep 20, 2017

Uh oh!

wking commented Sep 20, 2017

Uh oh!

wking commented Sep 20, 2017

Uh oh!

wking Sep 20, 2017 •

edited

Loading

Uh oh!

wking Sep 21, 2017

Uh oh!

wking commented Sep 21, 2017

Uh oh!

wking commented Sep 21, 2017

Uh oh!

xiekeyang commented Sep 21, 2017

Uh oh!

xiekeyang commented Sep 21, 2017

Uh oh!

xiekeyang commented Sep 21, 2017

Uh oh!

wking commented Sep 21, 2017

Uh oh!

xiekeyang commented Sep 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tools/refenginediscovery: Refactor Ref-Engine Discovery library API #29

tools/refenginediscovery: Refactor Ref-Engine Discovery library API #29

Uh oh!

Conversation

wking commented Sep 20, 2017

Uh oh!

wking commented Sep 20, 2017

Uh oh!

wking commented Sep 20, 2017

Uh oh!

wking Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

wking commented Sep 21, 2017

Uh oh!

wking commented Sep 21, 2017

Uh oh!

xiekeyang commented Sep 21, 2017

Uh oh!

xiekeyang commented Sep 21, 2017

Uh oh!

xiekeyang commented Sep 21, 2017

Uh oh!

wking commented Sep 21, 2017

Uh oh!

xiekeyang commented Sep 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wking Sep 20, 2017 •

edited

Loading