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

Remove requiredness of name attribute in ObjectMeta #85

Closed
wants to merge 3 commits into from

Conversation

arianvp
Copy link
Member

@arianvp arianvp commented Oct 19, 2019

It is only required for top-level objects . (E.g. Deployment, Pod, DaemonSet) but it is Optional when it is part of a compond object
(e.g. PodTemplateSpec inside a Deployment or DaemonSet)

See #8 (comment)
and #84 (comment)

It is only required for top-level objects .  (E.g. `Deployment`, `Pod`, `DaemonSet`) but it is Optional when it is part of a compond object
(e.g. `PodTemplateSpec` inside a `Deployment` or `DaemonSet`)


See #8 (comment)
and #84 (comment)
@arianvp
Copy link
Member Author

arianvp commented Oct 19, 2019

We could do something smart here: Generate ObjectMetaTopLevel and ObjectMeta types for the both usecases. E.g. a PodTemplateSpec will have type ObjectMeta with the field optional whilst Deployment will have ObjectMetaTopLevel with the field required.

However I think just making it optional is maybe the better option here. Thoughts?

Copy link
Contributor

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine. The type safety doesn't have to be perfect. For example, if we really went all in on type safety we'd have to fix fields like updateStrategy.type to be enums that enumerated all possible valid values. I think the right thing to do is to build a higher-level library on top of this one that provides stronger guarantees and keep the scope of this library to just auto-generating raw Dhall types

@arianvp
Copy link
Member Author

arianvp commented Oct 20, 2019

While I'm at it I should probably fix the examples to contain a selector

@akshaymankar
Copy link

@arianvp You missed the README.md :)

@arianvp
Copy link
Member Author

arianvp commented Jan 8, 2020

sorry this kind of got side-tracked =) i'll have another look at this soon

@ggilmore
Copy link

Any updates for this PR?

@Gabriella439
Copy link
Contributor

I can open up a new pull request against master with the same changes if nobody objects to the substance of this

Gabriella439 added a commit that referenced this pull request Apr 23, 2020
@Gabriella439
Copy link
Contributor

Alright, I have a new pull request out that supersedes this one: #115

@Gabriella439 Gabriella439 deleted the arianvp-patch-1 branch April 23, 2020 04:31
Gabriella439 added a commit that referenced this pull request Apr 24, 2020
TristanCacqueray pushed a commit to TristanCacqueray/dhall-haskell that referenced this pull request Jul 25, 2020
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