-
Notifications
You must be signed in to change notification settings - Fork 58
Minimal dispatch server based on knative #617
Minimal dispatch server based on knative #617
Conversation
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 a great start, some comments so far..
{{- define "ingress_with_external_auth" -}} | ||
{{- $ingress_enabled := default .Values.global.ingress.enabled .Values.ingress.enabled -}} | ||
{{- $ingress_enabled := .Values.ingress.enabled -}} |
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.
Nit: No need to define this variable, we can directly use .Values.ingress.enabled
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.
Thanks, yeah, I made a pass and removed all global refs.
config/300-function-manager.yaml
Outdated
@@ -14,3 +14,12 @@ spec: | |||
image: github.com/vmware/dispatch/cmd/dispatch-server | |||
args: | |||
- function-manager | |||
- --image-registry berndtj |
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.
No thanks ;). This can be ${KO_DOCKER_REPO}
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.
Oh... yeah, I'm not sure I want to keep the config/*.yaml files. Just one way to install. But that's for another commit
config/300-function-manager.yaml
Outdated
@@ -14,3 +14,12 @@ spec: | |||
image: github.com/vmware/dispatch/cmd/dispatch-server | |||
args: | |||
- function-manager | |||
- --image-registry berndtj | |||
- --sourceroot /store | |||
volumeMounts: |
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 thought this didn't work.
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.
Yeah, it doesn't :). I'm reverting all my changes to config, but like I said, I'd like to delete it
charts/dispatch/values.yaml
Outdated
insecure: false | ||
# Use https://index.docker.io/v1/ for dockerhub | ||
url: | ||
# Use 'echo -n "username" | base64' to generate this string |
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.
It will be nice if we base64 enc in helm
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.
Yeah, I agree. I didn't bother writing a helper to do so (or look to see if one already exists).
a11a27d
to
f50d70a
Compare
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.
LGTM but for the imageRegistry config
- "--host=0.0.0.0" | ||
- "--port={{ .Values.service.internalPort }}" | ||
- "--image-registry=berndtj" |
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.
.Values.registry.url
charts/dispatch/values.yaml
Outdated
#requests: | ||
# cpu: 100m | ||
# memory: 128Mi | ||
# Default values for function-manager. |
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.
Nit: Redundant Comments L4-L6
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.
Forgot this one. Fixed now. Should be all good to go
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.
LGTM
f50d70a
to
ca49023
Compare
Can simply build and execute functions * remove separate services/managers, go to a single dispatch server. - may still keep identity manager * remove all obsolete charts * remove old function manager (non-knative code)
ca49023
to
fc9e433
Compare
Can simply build and execute functions
server.