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

Go back to --omitNull instead of --omitEmpty #86

Closed
Gabriella439 opened this issue Oct 20, 2019 · 3 comments · Fixed by #110
Closed

Go back to --omitNull instead of --omitEmpty #86

Gabriella439 opened this issue Oct 20, 2019 · 3 comments · Fixed by #110

Comments

@Gabriella439
Copy link
Contributor

Gabriella439 commented Oct 20, 2019

... which would imply wrapping all of the non-required fields in Optional.

I'll admit that I originally proposed --omitEmpty as part of #46 but in retrospect I think it may have been a mistake, for a few reasons:

One example is the issue described in #78 caused by using --omitEmpty. There @ari-becker quoted this from the documentation:

A label selector is a label query over a set of resources. … An empty label selector matches all objects. A null label selector matches no objects.

... but when you use omitEmpty there is no way to distinguish a present but empty field (which means to match everything) from an absent field (which means to match nothing). They are semantically distinct!

Similarly, #77 was caused by the same problem: accidentally omitting a field that was a record where transitive fields were all empty (the default EmptyDirVolumeSource), and that was semantically not the same thing.

Also, more generally, Kubernetes is not the only configuration format that has issues like these. For example, I ran into a similar issue when working on encoding the Mergify configuration format in Dhall as part of this chapter of the dhall-manual. It has a delete_head_branch field that stores a value of type {} and with --omitEmpty there is no way to preserve that field.

The reason I'm bringing up non-Kubernetes examples is because I'm trying to slowly come up with best practices as part of writing the Dhall manual and it's becoming increasingly clear to me that --omitEmpty is an anti-pattern that leads to issues like these because it runs the risk of throwing away semantically significant information.

You also don't gain much these days since all that --omitEmpty buys you is omitting a few Somes. Now that we have the :: operator (which has higher precedence than Some) it's actually not that bad to just keep all the Somes for non-required fields. Also if the Somes were to really become problematic we could always revisit that in this issue:

@arianvp
Copy link
Member

arianvp commented Nov 11, 2019

I am convinced

@amarrella
Copy link
Collaborator

amarrella commented Jan 30, 2020

Now that dhall's default behaviour is to to omit nulls I think we should also reflect it here going back to using Optional (List _)) with a default of None instead of having List _ with a default of [].

This will help cover all the use cases. I have been bitten by this issue in EarnestResearch/dhall-packages#49 (but i can't remove the --omit-empty flag until dhall-kubernetes does) and we resorted to using a dummy value, but it's not optimal

Gabriella439 added a commit that referenced this issue Jan 30, 2020
Fixes #86

The motivation of this is to more accurately model the Kubernetes
API semantics by not auto-omitting empty fields.  This is because
a field set to `Some ([] : List T)` is not necessarily the same
as `None (List T)`.

This makes the typical case a bit more verbose (more `Some`s), but
the difference to the `./examples` shows that it's not too bad.
@Gabriella439
Copy link
Contributor Author

Fix is up here: #110

Gabriella439 added a commit that referenced this issue Jan 31, 2020
Fixes #86

The motivation of this is to more accurately model the Kubernetes
API semantics by not auto-omitting empty fields.  This is because
a field set to `Some ([] : List T)` is not necessarily the same
as `None (List T)`.

This makes the typical case a bit more verbose (more `Some`s), but
the difference to the `./examples` shows that it's not too bad.
drifterza pushed a commit to drifterza/zuul-operator that referenced this issue Apr 8, 2020
This change update the kubernetes binding to use the new
Optional types. The main consequence is that all the fields that
are optional needs to be prefixed with Some. This let us remove
the `--omit-empty` parameter resulting in cleaner resources where
we don't need to set a dummy emptyDir medium value.

See this issue for the details:
dhall-lang/dhall-kubernetes#86

Change-Id: I23a0a028909208cd58f57a6f07ee93090b3f3a1a
TristanCacqueray pushed a commit to TristanCacqueray/dhall-haskell that referenced this issue Jul 25, 2020
Fixes dhall-lang/dhall-kubernetes#86

The motivation of this is to more accurately model the Kubernetes
API semantics by not auto-omitting empty fields.  This is because
a field set to `Some ([] : List T)` is not necessarily the same
as `None (List T)`.

This makes the typical case a bit more verbose (more `Some`s), but
the difference to the `./examples` shows that it's not too bad.
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 a pull request may close this issue.

3 participants