-
Notifications
You must be signed in to change notification settings - Fork 101
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
Change to Go text/template #295
Conversation
cc @fabianbaier once this lands we'll need to coordinate during our release process updating all of our frameworks, and making others aware that v0.2.0 completely supersedes v0.1.0. We're pre-1.0 so there's obviously no expectation of stability here. cc-ing @mattj-io @yankcrime @guenter, this will affect any demos or presentations you do once the frameworks repo is updated to match. After that, you'll need to use v0.2.0 for anything. |
@@ -242,23 +242,23 @@ spec: | |||
apiVersion: apps/v1 | |||
kind: StatefulSet | |||
metadata: | |||
name: {{FRAMEWORK_NAME}} | |||
namespace: {{NAMESPACE}} | |||
name: {{ .FrameworkName }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These non-parameter varaibles should be layed out in documentation somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of KEP-9's Execution State section. I'll open a separate PR updating KEP-9 once the dust settles on that and we get your PR + new install method all in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Then, we also need to write actual docs based on that section)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the exact same thought as Tom 👍 should we maybe track it in an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked surprisingly small. Nice! :)
@@ -242,23 +242,23 @@ spec: | |||
apiVersion: apps/v1 | |||
kind: StatefulSet | |||
metadata: | |||
name: {{FRAMEWORK_NAME}} | |||
namespace: {{NAMESPACE}} | |||
name: {{ .FrameworkName }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the exact same thought as Tom 👍 should we maybe track it in an issue?
Closes #268 |
Frameworks have been updated kudobuilder/operators#19 |
This is basic, but works. There's a larger PR coming that starts to move other logic up into the engine package, such as assembling all of the values from a FrameworkVersion (to move that out of FV), but this is simple and gets people unblocked.