-
Notifications
You must be signed in to change notification settings - Fork 213
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
DRY out deployment and config #57
Conversation
0aae7f2
to
0f871c0
Compare
@@ -51,16 +51,18 @@ dashboard: | |||
type: ClusterIP | |||
image: | |||
repository: quay.io/reactiveops/fairwinds | |||
tag: dev-7263ff7e59b3809bf8ea5e7845261a9da14752c7 | |||
tag: master |
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.
Sweet
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 looks really great, thanks for all the improvements! One thing that didn't really fit in any comments below is that I think we likely don't want the webhook deployed by default like it is with deploy/all.yaml
. Maybe that means we should generate separate deploy/dashboard.yaml
and deploy/webhook.yaml
files? That doesn't necessarily have to be part of this PR though.
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 looking really great, thanks for all the improvements! Since there's been a lot of upstream changes, it looks like we ended up with a lot of merge commits in this PR, do mind rebasing/squashing those? We've been trying to use rebasing on this project as much as possible as a bit of an experiment in Git workflows.
dd175c8
to
00f3ca0
Compare
00f3ca0
to
0b3d93e
Compare
Rebased and ready to go. |
Fixes #53
deploy/all.yaml
anddeploy/helm/templates
There were a few inconsistencies between the chart and the all.yaml that this irons out. It fixes up things like volume names, port numbers, and default configs. The
all.yaml
is now just the output ofhelm template
and Circle now asserts that as well.When generating the
all.yaml
we use a newbasicLabels
option that removes all the standard helm labels in favor of just a simple labeling scheme.This also includes rok8s-scripts for docker build/push to benefit from better caching and changes the chart (and
all.yaml
) to use themaster
tag by default.Future things to handle:
all.yaml
pointing to the latest image that it works with (goes along with discussion about where to ultimately host the chart)