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

Expose more configuration through values.yaml files #6320

Merged
merged 2 commits into from
Sep 24, 2021

Conversation

jonstacks
Copy link
Contributor

What

This bumps the helm chart version and allows for changing more configuration values by overwriting the values.yaml or supplying --sets when installing/upgrading.

How

  • Changed the template that generates the full image path to consider global.imageRegistry. This makes it easier to use a private image registry for all images.
  • Exposed frontend configuration for fullstory, openreplay, storytime, and whether or not it is a demo.
  • Allow for changing log level independently for scheduler and server.
  • The helm chart can now be used to deploy different versions of Airbyte without having to bump the .Chart.appVersion and releasing a new version of the chart. To do this, you can override the values.yaml when installing with a partial like so:
version: 0.29.22-alpha
webapp:
  image:
    tag: 0.29.22-alpha
server:
  image:
    tag: 0.29.22-alpha
scheduler:
  image:
    tag: 0.29.22-alpha

Its a little verbose, but it allows changing all values independently if testing different configurations is desired.

@jrhizor jrhizor requested review from davinchia and jrhizor and removed request for davinchia September 20, 2021 18:15
@marcosmarxm
Copy link
Member

marcosmarxm commented Sep 22, 2021

@jonstacks we use bumpversion to change version in files. Not sure, but can we add the chart files to automatically change using bumpversion?

[bumpversion:file:kube/overlays/stable/kustomization.yaml]

@jonstacks
Copy link
Contributor Author

@marcosmarxm, sounds like a great idea to me. I will look into it!

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

Looks good, just some comments for my understanding.

## @param global.storageClass Global StorageClass for Persistent Volume(s)
##
global:
imageRegistry: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

curious: what does this default to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having global.imageRegistry: "" will use dockerhub.

Example of the image generated using the default:

helm template airbyte . -s templates/scheduler/deployment.yaml | grep "image:" 
        image: airbyte/scheduler:0.29.21-alpha

Example of overriding this to use a private registry:

helm template airbyte . --set global.imageRegistry="my.docker.registry" -s templates/scheduler/deployment.yaml | grep "image:"
        image: my.docker.registry/airbyte/scheduler:0.29.21-alpha

@@ -28,7 +28,7 @@ spec:
{{- end }}
containers:
- name: airbyte-pod-sweeper
image: "{{ .Values.podSweeper.image.repository }}:{{ .Values.podSweeper.image.tag}}"
image: {{ include "airbyte.podSweeperImage" . }}
Copy link
Contributor

Choose a reason for hiding this comment

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

why change this to use include instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah is this using the custom functions defined in _helpers.tpl?

Copy link
Contributor Author

@jonstacks jonstacks Sep 23, 2021

Choose a reason for hiding this comment

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

Yeah. I'm using a helper function from the bitnami common library chart to make it easier to support global.imageRegistry. Doing that in the helpers makes the main template a bit more readable IMO.

@TSkrebe
Copy link
Contributor

TSkrebe commented Nov 28, 2021

app.kubernetes.io/version labels are currently following Chart.appVersion. Shouldn't they follow Values.version if it's defined?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants