Refactor OSS Helm Charts #14794

xpuska513 · 2022-07-18T13:37:19Z

What

Describe what the change is solving
It helps to add screenshots if it affects the frontend.

Refactored helm chart and splitted it out into own separate charts as per helm best practices
Added CI workflow to publish new charts to ArtifactHub and it's own helm repository

How

Describe the solution
Refactoring done using Helm docs and help of community members
CI workflow made using git mkver

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Probable user impact:

Failure during upgrade due to misconfigured bitnami/minio chart, an README file will be added about mitigating this issue

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

CLAassistant · 2022-07-18T13:37:23Z

All committers have signed the CLA.

mkver.conf

charts/airbyte-pod-sweeper/Chart.yaml

davinchia · 2022-07-20T05:29:58Z

.github/workflows/publish-helm-charts.yml

+      - name: "Helm package main chart"
+        shell: bash
+        run: |
+          sleep 120


what is this sleep for?

Since in main chart I'm referencing charts from our new repo, we need to wait for changes to be indexed, that's why I added bash sleep there

can we add an echo telling the user that so they know they should expect to wait 2 minutes?

Yep, already added it

kyrylo, can we also add a comment here to make clear what this sleep is for

davinchia · 2022-07-20T05:30:59Z

.github/workflows/publish-helm-charts.yml

+          sleep 120
+          declare -a StringArray=("airbyte")
+          for val in ${StringArray[@]}; do
+            cd ./airbyte/charts/${val} && cat Chart.yaml && helm dep update && cd $GITHUB_WORKSPACE


to make sure i understand, helm dep update means we always update our chart dependencies before publishing?

We need to update and download dependencies of chart before publishing it(it packages everything into single tarball archive)

davinchia · 2022-07-20T05:34:56Z

mkver.conf

@@ -0,0 +1,80 @@
+tagPrefix: v


i'm not very familiar with mkver. can you write a short summary explaining this?

Basically mkver just doing semantic versioning for us, its using conventional commits for it. This config specifies settings for semantic gersioning like tag format, version format, files to automatically patch.
In short words, same thing as bumpversion. Actually we might use bumpversion instead of mkver, but need to tune it, so helm charts will have it's own versioning

davinchia · 2022-07-20T05:35:54Z

charts/airbyte/templates/_helpers.tpl

@@ -154,61 +142,50 @@ Add environment variables to configure database values
 {{- printf "jdbc:postgresql://%s:%s/%s" $host $port $dbName -}}
 {{- end -}}

-{{/*


what happened to these templates?

Strange that PullSecrets got deleted, let me check on that.
About temporal, since I moved it into it's own chart, this template won't work as expected, I did it in much faster manner, simply by explicit editing of the configmap.yaml file. (New services will have following format: {{ .Release.Name }}-{{ .Chart.Name }} e.g aribyte-worker

Reverted changes done to PullSecrets

charts/airbyte/Chart.yaml

davinchia

Looks great @xpuska513 !

I have some questions for my understanding since I haven't used some of these bits and it's been a while since I've set up helm publishing.

Questions:

It looks like because we added the charts to the bumpversion.cfg, upgrading airbyte will also upgrade the helm charts (assuming there aren't any changes to the helm charts) and there isn't more work to do. Is that right?
Can I confirm that hosting the helm charts in the Airbyte OSS repo means a user can do helm add repo and no longer needs to git checkout the airbyte repo to use our helm charts?
How are we authenticating to publish Helm charts? Am I missing something?
When publishing new helm charts, do the commits needs to follow the conventional commits due to the mkver configuration?

Since this is a big PR, I'm okay merging this as in and refining different parts further.

xpuska513 · 2022-07-20T06:00:48Z

It looks like because we added the charts to the bumpversion.cfg, upgrading airbyte will also upgrade the helm charts (assuming there aren't any changes to the helm charts) and there isn't more work to do. Is that right?

Can I confirm that hosting the helm charts in the Airbyte OSS repo means a user can do helm add repo and no longer needs to git checkout the airbyte repo to use our helm charts?

How are we authenticating to publish Helm charts? Am I missing something?

When publishing new helm charts, do the commits needs to follow the conventional commits due to the mkver configuration?

I believe it should upgrade only the appVersion parameter in chart.yaml
Yep, end user can simply add our repo via helm repo add airbyte https://aibytehq.github.io/helm-charts and use all our charts from there without clonning the repo
I'm clonning the helm-chart repo locally into github runner workspace, then running helm package that basically packages our chart and makes it usable, after that workflow copies that created .tgz arvhice to helm-charts repo and runs helm repo index which scans for new packaged charts and indexes the changed in index.yaml file which is basically help helm to understand the repo contents and then push the changes to helm-charts repo.
About it, let me do some another test in my fork so I can be sure that mkver won't touch main components semantic versioning

supertopher

Looks great, nothing but nits

thanks @xpuska513

supertopher · 2022-07-20T10:33:19Z

.github/workflows/publish-helm-charts.yml

+        run: |
+          declare -a StringArray=("airbyte-bootloader" "airbyte-server" "airbyte-temporal" "airbyte-webapp" "airbyte-pod-sweeper" "airbyte-worker")
+          for val in ${StringArray[@]}; do
+            cd ./airbyte/charts/${val} && helm dep update && cd $GITHUB_WORKSPACE


why are these commands linked with &&?

Just an old habit, like to do oneliner if we won't need to do anything in directory anymore

works for me, thanks for explaining

supertopher · 2022-07-20T10:35:20Z

.github/workflows/publish-helm-charts.yml

+      - name: "Helm package main chart"
+        shell: bash
+        run: |
+          sleep 120


can we add an echo telling the user that so they know they should expect to wait 2 minutes?

charts/airbyte-bootloader/templates/_helpers.tpl

charts/airbyte-bootloader/values.yaml

supertopher · 2022-07-20T10:48:02Z

charts/airbyte-bootloader/values.yaml

+## @param bootloader.resources.limits [object] The resources limits for the airbyte bootloader image
+## @param bootloader.resources.requests [object] The requested resources for the airbyte bootloader image
+resources:
+  ## Example:


nice to have the examples 👍

Also updated the docs using helm-docs

charts/airbyte-webapp/values.yaml

charts/airbyte/values.yaml

…re sensable. Update workflow

xpuska513 · 2022-07-20T12:13:51Z

@davinchia, so I updated the workflow, now it doesn't need mkversion at all and potentially it shouldn't screw up sem ver of your main applications.

xpuska513 · 2022-07-20T12:23:32Z

charts/airbyte-server/templates/deployment.yaml

+      {{- end }}
+      containers:
+      - name: airbyte-server-container
+        image: {{ printf "%s:%s" .Values.image.repository (default .Chart.AppVersion .Values.image.tag) }}


Now we also can forget about manually setting the image tag, it'll now use .Chart.AppVersion (that automatically should bump itself) as default value for image tag

supertopher

looks solid

supertopher · 2022-07-20T12:38:41Z

.github/workflows/publish-helm-charts.yml

+        run: |
+          declare -a StringArray=("airbyte-bootloader" "airbyte-server" "airbyte-temporal" "airbyte-webapp" "airbyte-pod-sweeper" "airbyte-worker")
+          for val in ${StringArray[@]}; do
+            cd ./airbyte/charts/${val} && helm dep update && cd $GITHUB_WORKSPACE


works for me, thanks for explaining

supertopher · 2022-07-20T12:39:41Z

charts/airbyte-server/templates/deployment.yaml

+      {{- end }}
+      containers:
+      - name: airbyte-server-container
+        image: {{ printf "%s:%s" .Values.image.repository (default .Chart.AppVersion .Values.image.tag) }}


xpuska513 · 2022-07-20T14:24:24Z

Bumped the version, as for me looks good.
@git-phu ?

git-phu

looks good to me, nice work!

charts/airbyte/values.yaml

* Add refactored helm charts * Add mkver.conf * Remove unused workflow debug info * Add newline * Add switch for rendering main chart templates * Update .bumpversion.cfg * Replace repository in main chart * Update helm-docs. Remove commented values. Replace placeholders to more sensable. Update workflow * Update workflow * Update workflow * Update workflow * Update workflow * Update workflow * Update test workflow * Update test workflow * Disable on push for main workflow. Add test action for test workflow * Delete test workflow. Update main workflow * Remove mkver.conf since its not used no more * Reference appVersion instead of manually set image tag * Revert image pull secrets * Bump appVersion to the latest * Update values.yaml

jan-benisek · 2022-08-15T13:25:27Z

charts/airbyte/Chart.yaml

+  - condition: temporal.enabled
+    name: temporal
+    repository: "https://airbytehq.github.io/helm-charts/"
+    version: placeholder


Are these placeholders supposed to be here @xpuska513 ? I just submitted an issue here: #15656

They're supposed, let me update the docs and explained a little bit more in the #15656

xpuska513 added 2 commits July 18, 2022 16:30

Add refactored helm charts

a03d5fc

Add mkver.conf

51ffcc4

github-actions bot added area/platform issues related to the platform kubernetes labels Jul 18, 2022

Remove unused workflow debug info

55c5a5f

xpuska513 requested review from davinchia, git-phu, supertopher and a team July 18, 2022 14:06

didistars328 reviewed Jul 18, 2022

View reviewed changes

mkver.conf Outdated Show resolved Hide resolved

Add newline

dbee96d

xpuska513 requested a review from didistars328 July 19, 2022 11:13

Add switch for rendering main chart templates

4faed2f

git-phu reviewed Jul 19, 2022

View reviewed changes

charts/airbyte-pod-sweeper/Chart.yaml Outdated Show resolved Hide resolved

Update .bumpversion.cfg

9cb7f74

xpuska513 requested a review from git-phu July 19, 2022 19:36

Replace repository in main chart

7c7de48

davinchia reviewed Jul 20, 2022

View reviewed changes

charts/airbyte/Chart.yaml Outdated Show resolved Hide resolved

davinchia reviewed Jul 20, 2022

View reviewed changes

charts/airbyte/Chart.yaml Outdated Show resolved Hide resolved

davinchia reviewed Jul 20, 2022

View reviewed changes

supertopher approved these changes Jul 20, 2022

View reviewed changes

xpuska513 added 3 commits July 20, 2022 14:39

Update helm-docs. Remove commented values. Replace placeholders to mo…

bf29d70

…re sensable. Update workflow

Update workflow

e5328d4

Update workflow

a5e6ece

xpuska513 added 7 commits July 20, 2022 14:48

Update workflow

4bff11b

Update workflow

589707c

Update workflow

bd6d264

Update test workflow

a0ce399

Update test workflow

eda1bdd

Disable on push for main workflow. Add test action for test workflow

2a3b979

Delete test workflow. Update main workflow

187f20e

xpuska513 requested review from davinchia and supertopher July 20, 2022 12:11

xpuska513 added 2 commits July 20, 2022 15:14

Remove mkver.conf since its not used no more

ac818ff

Reference appVersion instead of manually set image tag

2bfc7a5

xpuska513 commented Jul 20, 2022

View reviewed changes

supertopher approved these changes Jul 20, 2022

View reviewed changes

xpuska513 added 3 commits July 20, 2022 15:51

Revert image pull secrets

ca5d284

fix merge conflict

69bdb90

Bump appVersion to the latest

857db70

git-phu approved these changes Jul 20, 2022

View reviewed changes

git-phu reviewed Jul 20, 2022

View reviewed changes

charts/airbyte/values.yaml Outdated Show resolved Hide resolved

Update values.yaml

07eb5d0

git-phu approved these changes Jul 20, 2022

View reviewed changes

xpuska513 merged commit c7774a5 into airbytehq:master Jul 20, 2022

octavia-squidington-iii mentioned this pull request Jul 23, 2022

Bump Airbyte version from 0.39.37-alpha to 0.39.38-alpha #14976

Merged

jan-benisek mentioned this pull request Aug 15, 2022

Upgrading Airbyte - Helm chart has version: placeholder in some dependencies #15656

Closed

jan-benisek reviewed Aug 15, 2022

View reviewed changes

evantahler mentioned this pull request Aug 17, 2022

Helm chart: externalDatabase.password has no effect as it still retrieves from the secret #9979

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor OSS Helm Charts #14794

Refactor OSS Helm Charts #14794

xpuska513 commented Jul 18, 2022

CLAassistant commented Jul 18, 2022 •

edited

Loading

davinchia Jul 20, 2022

xpuska513 Jul 20, 2022

supertopher Jul 20, 2022

xpuska513 Jul 20, 2022

davinchia Jul 20, 2022

davinchia Jul 20, 2022

xpuska513 Jul 20, 2022

davinchia Jul 20, 2022

xpuska513 Jul 20, 2022

davinchia Jul 20, 2022

xpuska513 Jul 20, 2022

xpuska513 Jul 20, 2022

davinchia left a comment

xpuska513 commented Jul 20, 2022

supertopher left a comment

supertopher Jul 20, 2022

xpuska513 Jul 20, 2022

supertopher Jul 20, 2022

supertopher Jul 20, 2022

supertopher Jul 20, 2022

xpuska513 Jul 20, 2022

xpuska513 commented Jul 20, 2022

xpuska513 Jul 20, 2022

supertopher Jul 20, 2022

supertopher left a comment

supertopher Jul 20, 2022

supertopher Jul 20, 2022

xpuska513 commented Jul 20, 2022

git-phu left a comment

jan-benisek Aug 15, 2022

xpuska513 Aug 15, 2022

Refactor OSS Helm Charts #14794

Refactor OSS Helm Charts #14794

Conversation

xpuska513 commented Jul 18, 2022

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

CLAassistant commented Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia left a comment

Choose a reason for hiding this comment

xpuska513 commented Jul 20, 2022

supertopher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpuska513 commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supertopher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpuska513 commented Jul 20, 2022

git-phu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jul 18, 2022 •

edited

Loading