-
Notifications
You must be signed in to change notification settings - Fork 687
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
Revert #410 #557
Milestone
Comments
Cross-linking earlier discussion in #553.
|
@philips We should not emit fields that contain zero-values. It's wasteful and distracting. There are certain exceptions, but, in general, use of |
On Mon, Feb 06, 2017 at 02:20:19PM -0800, Stephen Day wrote:
There are certain exceptions, but, in general, use of `omitempty`
should be the rule.
If the exceptions are “when the zero-value is valid” (based on [1]).
Then:
$ git grep '^ [A-Z]' specs-go/ | grep -v omitempty
turns up the following fields (in master, as of ed18de1) which could
pick up omitempty:
* RootFS.type (REQUIRED, MUST be ‘layers’)
* RootFS.DiffIDs (REQUIRED, corresponsed to manifest's ‘layers’ which
REQUIRES at least one entry with #407 in flight).
* Image.Architecture (REQUIRED, empty string not explicitly illegal,
but it probably should be).
* Image.OS (REQUIRED, empty string not explicitly illegal, but it
probably should be).
* Descriptor.Digest (REQUIRED, empty string missing algorithm,
separator, and hash).
* ImageLayout.Version (REQUIRED, empty string will never be valid
unless image-spec cuts an empty-string release).
* Manifest.Layers (REQUIRES at least one entry with #407 in flight).
* Platform.Architecture (REQUIRED, empty string not explicitly
illegal, but it probably should be).
* Platform.OS (REQUIRED, empty string not explicitly illegal, but it
probably should be).
* Versioned.SchemaVersion (REQUIRED, must be 2).
I still don't see a point to adding omitempty to a REQUIRED property,
but if that's what you want to do, the above properties are probably
what you'd do it to.
Manifest.Manifests is REQUIRED, although without a requirement to have
at least one value I'm not sure why. In any case, as long as it's
required and allows an empty array we don't want ‘omitempty’ there.
[1]: #553 (comment)
|
I'm sorry but I misread #410. For the most part, we should be using Closing this now. Apologies. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
#410
omitempty
has nothing to do with REQUIRED vs OPTIONAL fields.The text was updated successfully, but these errors were encountered: