Skip to content

Add options field to CRDs#55

Merged
sgraband merged 2 commits intomainfrom
addOptions
Apr 18, 2024
Merged

Add options field to CRDs#55
sgraband merged 2 commits intomainfrom
addOptions

Conversation

@sgraband
Copy link
Contributor

This field can be used by adopters to pass arbitrary data to the operator.
Field is added to all three CRDs for maximum configurability.
Bumped version number to Session.v1beta8, Workspace.v1beta5 and AppDefinition.v1beta10.

Main Repo PR: https://github.com/eclipsesource/theia-cloud/pull/293

This field can be used by adopters to pass arbitrary data to the operator.
Field is added to all three CRDs for maximum configurability.
Bumped version number to `Session.v1beta8`, `Workspace.v1beta5` and `AppDefinition.v1beta10`.
Copy link
Contributor

@jfaltermeier jfaltermeier 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 we miss bumping the chart version of the crds.
Besides that I have one question.
The current options field allows to add one object with arbitrary integer or string properties, if I understand this correctly.

Would an array of objects that have a key/id property + allows any further integer and string properties make sense as well?
Then for each customization users could add an options object with a known id of the customization + the required values.
This might be a bit more tidy in the long run rather than having a flat list. But maybe this makes it more complicated in other areas

@sgraband
Copy link
Contributor Author

I bumped the chart version.

I see your point, but believe that adoptors that need this could simply prefix the keys with a customization string and a ..
This would keep things easier in the code base and we could give this tip in the documentation.

If this is nesting is requested a lot we can still think about it. WDYT?

Copy link
Contributor

@jfaltermeier jfaltermeier left a comment

Choose a reason for hiding this comment

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

Fine for me

@sgraband sgraband merged commit 92d88b5 into main Apr 18, 2024
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.

2 participants