Skip to content

build: support --format (pb and json) - #52

Closed
AkihiroSuda wants to merge 1 commit into
containerd:masterfrom
AkihiroSuda:jsonpb
Closed

AkihiroSuda wants to merge 1 commit into
containerd:masterfrom
AkihiroSuda:jsonpb

Conversation

@AkihiroSuda

@AkihiroSuda AkihiroSuda commented Mar 7, 2017 •

Copy link
Copy Markdown
Member

This PR implements continuity build --format.
Fix #46

e.g.

$ continuity build --format application/vnd.continuity.manifest.v0+json .
$ continuity build --format application/vnd.continuity.manifest.v0+pb .

alias:

$ continuity build --format json .
$ continuity build --format pb .

The JSON format is implemented using jsonpb.Marshaller{EnumAsInts: false, EmitDefaults: false, OrigName: false}. https://godoc.org/github.com/golang/protobuf/jsonpb#Marshaler

JSON format would be useful when a continuity manifest is included in an OCI image.

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

@AkihiroSuda

AkihiroSuda commented Mar 14, 2017 •

Copy link
Copy Markdown
Member Author

example output (after jq)

{
  "resource": [
    {
      "path": [
        "/a"
      ],
      "uid": "1001",
      "gid": "1001",
      "mode": 2147484157
    },
    {
      "path": [
        "/a/b"
      ],
      "uid": "1001",
      "gid": "1001",
      "mode": 436,
      "size": "476",
      "digest": [
        "sha256:ef290472c423c80ce142a1617ac556e5935f02b30db1c28c8ed38c15b62e9e3e"
      ]
    }
  ]
}

@dmcgowan

Copy link
Copy Markdown
Member

I think this is a reasonable addition. I am wondering though if we want to keep the existing package interface the same for Marshal and just have a MarshalJSON and same for unmarshal. Seems weird to me to have the marshaling functions take in a media type.

@AkihiroSuda

Copy link
Copy Markdown
Member Author

@dmcgowan thank you, updated PR

@AkihiroSuda

Copy link
Copy Markdown
Member Author

@stevvooe PTAL if you have a time? 😃

@stevvooe

Copy link
Copy Markdown
Member

@AkihiroSuda Could we not emit fields when they are empty? "xattr": {} is an example.

@AkihiroSuda

AkihiroSuda commented Mar 31, 2017 •

Copy link
Copy Markdown
Member Author

@stevvooe

It should have been already omitted, but due to a bug in protobuf, it is not actually omitted 😭

I confirmed the following patch for protobuf successfully omits empty xattr map.

diff --git a/jsonpb/jsonpb.go b/jsonpb/jsonpb.go
index 82c6162..9999705 100644
--- a/jsonpb/jsonpb.go
+++ b/jsonpb/jsonpb.go
@@ -188,6 +188,10 @@ func (m *Marshaler) marshalObject(out *errWriter, v proto.Message, indent, typeU
 
                if !m.EmitDefaults {
                        switch value.Kind() {
+                       case reflect.Map:
+                               if len(value.MapKeys()) == 0 {
+                                       continue
+                               }
                        case reflect.Bool:
                                if !value.Bool() {
                                        continue

I'll open PR to protobuf later.
( EDIT: opened golang/protobuf#327 )

@AkihiroSuda

Copy link
Copy Markdown
Member Author

protobuf maintainers seems unresponsive to golang/protobuf#327, do we need to wait for golang/protobuf#327 ?

@AkihiroSuda

Copy link
Copy Markdown
Member Author

@stevvooe

protobuf maintainers are still unresponsive to my PR about omitempty (golang/protobuf#327), but I think lack of omitempty is not a critical issue.

Can you please consider merging this PR as-is?

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda

AkihiroSuda commented May 24, 2017 •

Copy link
Copy Markdown
Member Author

rebased, and omitempty issue has been resolved in golang/protobuf@7a211bc

@stevvooe

Copy link
Copy Markdown
Member

@AkihiroSuda Has that change made it to gogo/protobuf?

@AkihiroSuda

Copy link
Copy Markdown
Member Author

Seems not yet

@AkihiroSuda

Copy link
Copy Markdown
Member Author

opened PR gogo/protobuf#296

@awalterschulze

Copy link
Copy Markdown

Its merged. Sorry for the delay.

@AkihiroSuda

Copy link
Copy Markdown
Member Author

thank you

@stevvooe

stevvooe commented Jun 2, 2017

Copy link
Copy Markdown
Member

@awalterschulze As always, thanks for the great support!

@stevvooe

stevvooe commented Jun 2, 2017

Copy link
Copy Markdown
Member

@AkihiroSuda Could you update the example output?

@stevvooe

stevvooe commented Jun 2, 2017

Copy link
Copy Markdown
Member

@AkihiroSuda Also, should probably rename the path field to paths. ;)

@AkihiroSuda

Copy link
Copy Markdown
Member Author

Also, should probably rename the path field to paths. ;)

let me do that in another PR

@AkihiroSuda

Copy link
Copy Markdown
Member Author

update the example output.
Is this mergeable? 😃

@stevvooe

Copy link
Copy Markdown
Member

@AkihiroSuda I think so, but we let's not build anything permanent on it yet.

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