Make the Platform field actually optional.

[vishvananda]

The spec states that platform is an optional field for the items in the
image index. Pull request #607 added omitempty to the field, but
omitempty only works for types with a zero value. Because Platform is a
struct, it will never be omitted. The platform field should instead be a
pointer so that it can be properly omitted when it has no value.
Currently, serializing an index.json without populating platform leads to
something like the following:

{
  "schemaVersion": 2,
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest":
"sha256:d73e0d70cb05a373b5a666ba608139624d90a88d0155d2ade06a6001a27cd8d5",
      "size": 348,
      "platform": {
        "architecture": "",
        "os": ""
      }
    }
  ]
}

With the change in this patch, leaving platform as nil will cause it to
be omitted as expected.

Signed-off-by: Vishvananda Ishaya Abrams vishvananda@gmail.com

[stevvooe]

This is actually an inconsistency in encoding/json implementation, described in golang/go#11939. An empty Platform{} should be considered empty but apparently zero-valued structs are not empty. Looks like this is similar to #626, as well. Disappointing.

Note that Platform.OS and Platform.Architecture should also have omitempty.

(And, like I've said before, omitempty has nothing to do with optional or required. It is only there to manage Go's json output.)

The main problem with this change is that once we do it, it will always be a pointer.

[jonboulle]

omitempty has nothing to do with optional or required.

Since we're working with Go, it's always appropriate for optional fields to be marked omitempty but never really appropriate for required fields to be so marked. So while we should be careful to avoid equivalence (or presuming that this tag provides validation), there's a relation.

As this (and the other referenced) issues show, a broken "omitempty" can have the dangerous consequence of inadvertently setting an optional property to a meaningful value.

[jonboulle]

Thanks to this golang tragédie comique, I'm in favour of going the pointer route for all optional properties.

[wking]

Thanks to this golang tragédie comique, I'm in favour of going the pointer route for all optional properties.

All optional properties? Or just optional structs? I think the latter is all we need, and will be less painful to work with in Go than having pointers for *all* optional properties (i.e. including strings and such as well as structs).

[jonboulle]

I would argue for all, because a) consistency, and b) it would prevent the kind of issue we've seen here, where a native type (with an empty value) is changed to a non-native type without empty values - which one might expect to be more commonplace if we introduce more type validation. But if folks feel that's too much additional complexity in working with these things I could be talked down.

[wking]

On Fri, Mar 31, 2017 at 09:54:29AM -0700, Jonathan Boulle wrote: I would argue for all…

I like the consistency of this as well, but runtime-spec was initially (nominally) aiming using pointers for all optional properties [1] and ended up reversing that position later [2]. If we decide to go with pointers for all optional properties, I'd recommend making sure all the maintainers are on-board first to avoid similar waffling. [1]: https://github.com/opencontainers/runtime-spec/pull/287/files#diff-3c39f2f23cfc3be285840b89a9685e8cR13 [2]: https://github.com/opencontainers/runtime-spec/pull/656/files#diff-3c39f2f23cfc3be285840b89a9685e8cL19

[stevvooe]

never really appropriate for required fields to be so marked.

This isn't really accurate unless the empty value is somehow valid. Adding omitempty doesn't protect you from generating json that is missing a required field, especially if the field requires an actual value. The default should be to always have omitempty unless it prevents us from generating spec compliant JSON.

Thanks to this golang tragédie comique, I'm in favour of going the pointer route for all optional properties.

This is going much too far. For example, it doesn't make sense to have a pointer to a slice. There are other properties where the empty value is sufficient for detecting presence. Otherwise, we end up paying the cost of pointers and nil checks where they are not needed, complicating creation and consumption.

Let's avoid hard and fast rules in response to minor gotchas. There are a small number of fields and analyzing them on a case by case basis takes two seconds. We will use pointers where they are needed. It is really not that hard.

LGTM

[jonboulle]

[...] It is really not that hard.

Come on, there's really no need to be so antagonistic. I already proffered I would be willing to compromise in my earlier comment. (Also, one could quite reasonably argue that if it's such a simple issue we would not still be facing these questions and issues years after they're first noticed.)

LGTM, let's move forward.

[stevvooe]

Also, one could quite reasonably argue that if it's such a simple issue we would not still be facing these questions and issues years after they're first noticed.

Before making new rules about the way things are done, we should make sure the existing processes are working properly. It seems like both cases (#633 and this one) could have been caught by proper golden master testing and validation.