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

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

Closed
wants to merge 1 commit into from

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Mar 7, 2017

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 [email protected]

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Mar 14, 2017

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
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
Member Author

@dmcgowan thank you, updated PR

@AkihiroSuda
Copy link
Member Author

@stevvooe PTAL if you have a time? 😃

@stevvooe
Copy link
Member

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

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Mar 31, 2017

@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
Member Author

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

@AkihiroSuda
Copy link
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?

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented May 24, 2017

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

@stevvooe
Copy link
Member

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

@AkihiroSuda
Copy link
Member Author

Seems not yet

@AkihiroSuda
Copy link
Member Author

opened PR gogo/protobuf#296

@awalterschulze
Copy link

Its merged. Sorry for the delay.

@AkihiroSuda
Copy link
Member Author

thank you

@stevvooe
Copy link
Member

stevvooe commented Jun 2, 2017

@awalterschulze As always, thanks for the great support!

@stevvooe
Copy link
Member

stevvooe commented Jun 2, 2017

@AkihiroSuda Could you update the example output?

@stevvooe
Copy link
Member

stevvooe commented Jun 2, 2017

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

@AkihiroSuda
Copy link
Member Author

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

let me do that in another PR

@AkihiroSuda
Copy link
Member Author

update the example output.
Is this mergeable? 😃

@stevvooe
Copy link
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