Skip to content

runtime-config: Drop optional 'options' when empty #123

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
Closed
wking wants to merge 1 commit into from
Closed

runtime-config: Drop optional 'options' when empty #123

wking wants to merge 1 commit into from

Conversation

@wking
Copy link
Contributor

@wking wking commented Aug 29, 2015

The field is optional, so why bother giving an empty value?

I'd suggested this after looking over #120, but this change is
orthogonal to that PR's string→array change.

@wking
runtime-config: Drop optional 'options' when empty
d4d246e 
The field is optional, so why bother giving an empty value?

Signed-off-by: W. Trevor King <[email protected]>
@philips
Copy link
Contributor

philips commented Aug 29, 2015

eh, I don't see how it maters either way. I am just going to close this.

@philips philips closed this Aug 29, 2015
@wking
Copy link
Contributor Author

wking commented Aug 30, 2015

On Sat, Aug 29, 2015 at 09:35:04AM -0700, Brandon Philips wrote:

eh, I don't see how it maters either way. I am just going to close
this.

I think giving explicit empty values for optional parameters is
useless noise, when the examples are trying to demonstrate the use of
the other fields. But yeah, it's not a big deal either way. If you
want to keep the explicitly empty values, I'll survive ;).

wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Sep 27, 2015
@wking
proto: Flesh out config.json
d8c80ca 
So it matches (modulo key order) our examples from the
runtime-config.md series.  To get as much matching up as possible,
I've converted externally-visible runtime_config.proto entries from
under_scores to camelCase.  We're still not matching some cases well,
and I've interspersed my notes on why in the diff:

  $ diff -u <(cat config.json runtime.json) <(./example_cpp | jq .)
  --- /dev/fd/63	2015-09-27 00:04:18.464247761 -0700
  +++ /dev/fd/62	2015-09-27 00:04:18.463247761 -0700
  @@ -34,43 +34,9 @@
     }
   }
   {
  -  "mounts": {
  -    "proc": {
  -      "type": "proc",
  -      "source": "proc",
  -      "options": []
  -    },
  -    ...
  -  },
  +  "mounts": [
  +    {}
  +  ],

Protobuf doesn't like objects with arbitrary keys (or at least we
haven't found a way to set that up).  So we probably need a
pre-processer to convert mount entries into:

  "mounts": [
    {
      "key": "proc",
      "value": {
        "type": "proc",
        "source": "proc",
        "options": []
      },
    }
  ]

You can do that with jq [1], and that makes the mount difference much
smaller (sysctl need the same change):

  $ mv runtime.json runtime.json-orig
  $ jq '.mounts |= to_entries | .linuxx.sysctl |= to_entries' <runtime.json-orig >runtime.json
  $ diff -u <(cat config.json runtime.json) <(./example_cpp | jq .)
  --- /dev/fd/63  2015-09-27 00:15:56.656282533 -0700
  +++ /dev/fd/62  2015-09-27 00:15:56.656282533 -0700
  @@ -78,8 +78,7 @@
         "key": "proc",
         "value": {
           "type": "proc",
  -        "source": "proc",
  -        "options": []
  +        "source": "proc"
         }
       }
     ],
  ...

The difference here is that protobuf isn't serializing default values,
so it drops the empty array [2] (which I'd suggested we do anyway [3] ;)

  ...
       "rlimits": [
         {
           "type": "RLIMIT_NPROC",
  -        "soft": 1024,
  -        "hard": 102400
  +        "hard": "102400",
  +        "soft": "1024"
         }
       ],
  ...

Protobuf uses strings when writing 64-bit-wide numbers [2], but it
reads both numbers and strings, so this isn't a big deal.

  @@ -198,63 +171,44 @@
       "devices": [
         {
           "path": "/dev/random",
  -        "type": "c",
  -        "major": 1,
  -        "minor": 8,
  -        "permissions": "rwm",
  -        "fileMode": 666,
  -        "uid": 0,
  -        "gid": 0
  +        "major": "1",
  +        "minor": "8",
  +        "permissions": "rwm",
  +        "fileMode": 666
         },
  ...

This has some of the default-dropping and number-string issues
mentioned earlier, as well as the effect of runtime_config.proto's:

  // TODO(vbatts) ensure int32 is fine here, instead of golang's rune
  optional int32 type = 2;

[1]: https://stedolan.github.io/jq/
[2]: https://developers.google.com/protocol-buffers/docs/proto3#json
[3]: opencontainers#123

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Sep 27, 2015
@wking
proto: Flesh out runtime.json (and pass it through example_cpp)
5f422ee 
So it matches (modulo key order) our examples from the
runtime-config.md series.  To get as much matching up as possible,
I've converted externally-visible runtime_config.proto entries from
under_scores to camelCase.  We're still not matching some cases well,
and I've interspersed my notes on why in the diff:

  $ diff -u <(cat config.json runtime.json) <(./example_cpp | jq .)
  --- /dev/fd/63	2015-09-27 00:04:18.464247761 -0700
  +++ /dev/fd/62	2015-09-27 00:04:18.463247761 -0700
  @@ -34,43 +34,9 @@
     }
   }
   {
  -  "mounts": {
  -    "proc": {
  -      "type": "proc",
  -      "source": "proc",
  -      "options": []
  -    },
  -    ...
  -  },
  +  "mounts": [
  +    {}
  +  ],

Protobuf doesn't like objects with arbitrary keys (or at least we
haven't found a way to set that up).  So we probably need a
pre-processer to convert mount entries into:

  "mounts": [
    {
      "key": "proc",
      "value": {
        "type": "proc",
        "source": "proc",
        "options": []
      },
    }
  ]

You can do that with jq [1], and that makes the mount difference much
smaller (sysctl need the same change):

  $ mv runtime.json runtime.json-orig
  $ jq '.mounts |= to_entries | .linuxx.sysctl |= to_entries' <runtime.json-orig >runtime.json
  $ diff -u <(cat config.json runtime.json) <(./example_cpp | jq .)
  --- /dev/fd/63  2015-09-27 00:15:56.656282533 -0700
  +++ /dev/fd/62  2015-09-27 00:15:56.656282533 -0700
  @@ -78,8 +78,7 @@
         "key": "proc",
         "value": {
           "type": "proc",
  -        "source": "proc",
  -        "options": []
  +        "source": "proc"
         }
       }
     ],
  ...

The difference here is that protobuf isn't serializing default values,
so it drops the empty array [2] (which I'd suggested we do anyway [3] ;)

  ...
       "rlimits": [
         {
           "type": "RLIMIT_NPROC",
  -        "soft": 1024,
  -        "hard": 102400
  +        "hard": "102400",
  +        "soft": "1024"
         }
       ],
  ...

Protobuf uses strings when writing 64-bit-wide numbers [2], but it
reads both numbers and strings, so this isn't a big deal.

  @@ -198,63 +171,44 @@
       "devices": [
         {
           "path": "/dev/random",
  -        "type": "c",
  -        "major": 1,
  -        "minor": 8,
  -        "permissions": "rwm",
  -        "fileMode": 666,
  -        "uid": 0,
  -        "gid": 0
  +        "major": "1",
  +        "minor": "8",
  +        "permissions": "rwm",
  +        "fileMode": 666
         },
  ...

This has some of the default-dropping and number-string issues
mentioned earlier, as well as the effect of runtime_config.proto's:

  // TODO(vbatts) ensure int32 is fine here, instead of golang's rune
  optional int32 type = 2;

[1]: https://stedolan.github.io/jq/
[2]: https://developers.google.com/protocol-buffers/docs/proto3#json
[3]: opencontainers#123

Signed-off-by: W. Trevor King <[email protected]>
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

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wking @philips