Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

make resource required and also fix examples to be consistent with spec #435

Merged
merged 2 commits into from
Nov 14, 2019

Conversation

wonderflow
Copy link
Member

@wonderflow wonderflow commented Oct 30, 2019

though discussion is still undergoing(oam-dev/spec#218) , we could align resource requirement with OAM spec v1.0.0-alpha.1, so we won't make confusion or misleading。 fixes #406 fixes #364

@wonderflow wonderflow changed the title Resource make resource required and also fix examples to be consistent with spec Oct 30, 2019
@technosophos
Copy link
Contributor

I still think this is a bad idea to have something outside of the orchestrator be the thing that decides on defaults. We have already seen this backfire in practice.

@wonderflow
Copy link
Member Author

@technosophos what if remove the default ? we make it real required in spec?

@suhuruli
Copy link
Contributor

suhuruli commented Nov 1, 2019

@technosophos , thoughts after @wonderflow's comment?

@resouer
Copy link
Member

resouer commented Nov 7, 2019

My thoughts on this: oam-dev/spec#218 (comment), in short:

  1. resources section could be optional but needed for claiming resource hints.
  2. resource trait is the recommended way for ops to step in.
  3. default value is a bad practice but it's a must for some platforms.

@wonderflow
Copy link
Member Author

So what do you think about this PR now? @technosophos

Copy link
Contributor

@technosophos technosophos left a comment

Choose a reason for hiding this comment

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

I am okay with this change.

Copy link
Member

@hongchaodeng hongchaodeng left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: Component Schematic concepts article misleading table The resources field should be required
6 participants