-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat: ZENKO-779 cloudserver refactor #205
Conversation
f06a207
to
b14d4ae
Compare
secret: | ||
secretName: {{ template "cloudserver-front.fullname" . }}-proxy | ||
{{- end }} | ||
{{- end}} |
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.
Newline 👆
1c977fd
to
c7a2eac
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.
Did something prevent git's heuristic to detect the rename of the charts/cloudserver/template/deployment.yaml
file?
It would be nice to not disconnect the history.
value: "{{ .Values.proxy.http }}" | ||
- name: HTTP_PROXY | ||
value: "{{ .Values.proxy.http }}" | ||
- name: https_proxy |
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.
why do we set https_proxy
and HTTPS_PROXY
if .Values.proxy.http
is set?
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 remember there was a reason for this. @giacomoguiulfo Might have more insight
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.
There are still ~20 references to cloudserver-front
in the repo. You need to do a rebase.
value: "{{ .Release.Name }}-cloudserver,{{ .Values.endpoint }}" | ||
- name: HEALTHCHECKS_ALLOWFROM | ||
value: "{{ .Values.allowHealthchecksFrom }}" | ||
{{- if .Values.storageLimit.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.
This should be a child of orbit -> .Values.orbit...
@@ -1,7 +1,7 @@ | |||
apiVersion: v1 | |||
kind: Pod | |||
metadata: | |||
name: {{ template "cloudserver-front.fullname" . }}-test | |||
name: {{ template "cloudserver.fullname" . }}-test |
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.
Since you are modifying this value already, a better name will be:
{{ template "cloudserver.fullname" . }}-service-test
charts/cloudserver/values.yaml
Outdated
# This is a YAML-formatted file. | ||
# Declare variables to be passed into your templates. | ||
|
||
orbit: | ||
enabled: true | ||
endpoint: https://api.zenko.io | ||
pushEndpoint: https://push.api.zenko.io | ||
mode: push | ||
managerMode: push | ||
workerMode: poll | ||
# When 'orbit.enabled' is 'true', these aren't used, please use | ||
# https://zenko.io to manage your deployment | ||
storageLimit: |
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.
storageLimit
should be part of orbit
.
@ploki I think the changes in the cloudserver manager commit |
90c3c99
to
698de82
Compare
This allows for cloudserver to have multiple replicas plus a manager that can still properly report metrics back to Orbit.
37a91f0
to
3deebf4
Compare
Upgrading should be seamless as long as all the old chart tar balls are deleted from Zenko/charts/zenko/charts and helm dependency build zenko is re-ran.
3deebf4
to
7b23822
Compare
This refactor abstracts some of the environment vars that are needed for both the manager and cloudserver workers. While the manager is a separate deployment, the "cloudserver" service still includes the manager in the load balancing since it is a fully capable cloudserver instance.
Regarding upgradability: Upgrading should be seamless as long as all the old chart tar balls are deleted from
Zenko/charts/zenko/charts
andhelm dependency build zenko
is re-ran.