Skip to content
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

Merged
merged 6 commits into from
Jul 21, 2018

Conversation

ssalaues
Copy link
Contributor

@ssalaues ssalaues commented Jul 20, 2018

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 and helm dependency build zenko is re-ran.

@ssalaues ssalaues force-pushed the feature/ZENKO-779-cloudserver-refactor branch from f06a207 to b14d4ae Compare July 20, 2018 22:54
@giacomoguiulfo giacomoguiulfo changed the title Feature/zenko 779 cloudserver refactor feat: zenko 779 cloudserver refactor Jul 20, 2018
secret:
secretName: {{ template "cloudserver-front.fullname" . }}-proxy
{{- end }}
{{- end}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Newline 👆

@giacomoguiulfo giacomoguiulfo changed the title feat: zenko 779 cloudserver refactor feat: ZENKO-779 cloudserver refactor Jul 20, 2018
@ssalaues ssalaues force-pushed the feature/ZENKO-779-cloudserver-refactor branch 2 times, most recently from 1c977fd to c7a2eac Compare July 20, 2018 23:21
Copy link
Contributor

@ploki ploki left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

@giacomoguiulfo giacomoguiulfo left a 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 }}
Copy link
Contributor

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
Copy link
Contributor

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

# 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:
Copy link
Contributor

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.

@ssalaues
Copy link
Contributor Author

@ploki I think the changes in the cloudserver manager commit b7180c23b38af85cccd52a808850d198eaf90983 were too big that caused git to think it is an entirely new file.

@ssalaues ssalaues force-pushed the feature/ZENKO-779-cloudserver-refactor branch 5 times, most recently from 90c3c99 to 698de82 Compare July 21, 2018 02:02
Salim added 2 commits July 21, 2018 11:31
This allows for cloudserver to have multiple replicas plus a manager that can still properly report metrics back to Orbit.
@ssalaues ssalaues force-pushed the feature/ZENKO-779-cloudserver-refactor branch 2 times, most recently from 37a91f0 to 3deebf4 Compare July 21, 2018 19:33
Salim added 4 commits July 21, 2018 12:59
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.
@ssalaues ssalaues force-pushed the feature/ZENKO-779-cloudserver-refactor branch from 3deebf4 to 7b23822 Compare July 21, 2018 19:59
@ssalaues ssalaues merged commit 12fc959 into master Jul 21, 2018
@ssalaues ssalaues deleted the feature/ZENKO-779-cloudserver-refactor branch July 21, 2018 20:24
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.

5 participants