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

DRY out deployment and config #57

Merged
merged 1 commit into from
Apr 22, 2019
Merged

DRY out deployment and config #57

merged 1 commit into from
Apr 22, 2019

Conversation

coreypobrien
Copy link
Contributor

@coreypobrien coreypobrien commented Apr 12, 2019

Fixes #53

  • Sync everything in deploy/all.yaml and deploy/helm/templates
  • Handle certs/secret for webhook
  • Ensure things stay in sync

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 of helm template and Circle now asserts that as well.

When generating the all.yaml we use a new basicLabels 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 the master tag by default.

Future things to handle:

  • don't rebuild the image when we release
  • figure out a better way to keep all.yaml pointing to the latest image that it works with (goes along with discussion about where to ultimately host the chart)

@coreypobrien coreypobrien force-pushed the drydeployandconfig branch 6 times, most recently from 0aae7f2 to 0f871c0 Compare April 16, 2019 16:34
@@ -51,16 +51,18 @@ dashboard:
type: ClusterIP
image:
repository: quay.io/reactiveops/fairwinds
tag: dev-7263ff7e59b3809bf8ea5e7845261a9da14752c7
tag: master
Copy link

Choose a reason for hiding this comment

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

Sweet

@coreypobrien coreypobrien changed the title WIP: DRY out deployment and config DRY out deployment and config Apr 16, 2019
Copy link
Contributor

@robscott robscott left a 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.

README.md Outdated Show resolved Hide resolved
deploy/all.yaml Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
deploy/helm/fairwinds/templates/secret.yaml Show resolved Hide resolved
Copy link
Contributor

@robscott robscott left a 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.

@coreypobrien
Copy link
Contributor Author

Rebased and ready to go.

@robscott robscott merged commit 6939416 into master Apr 22, 2019
@robscott robscott deleted the drydeployandconfig branch April 22, 2019 14:19
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.

Use helm template to generate manifests
3 participants