WIP: Adding Linting and Testing on all committs #352

edmondop · 2023-01-22T01:39:03Z

What was changed

Addresses #343

Getting the build to pass seems to be blocked by #182 but I don't think disabling hooks is appropriate:

master...BAXUSNFT:temporal-helm-charts:master#diff-5f8eb04d49dd7caffd33da659dd7bae84435c114a6255319b2965dcf6a2536ab

As you see from my fork https://github.com/edmondop/helm-charts/actions/runs/3988830053/jobs/6840499585 , the install fails because the default keyspace is never ready.

underrun · 2023-01-26T14:26:49Z

.github/workflows/ci.yml

+      - name: Set up Python
+        uses: actions/setup-python@v2
+        with:
+          python-version: 3.7


does this require a really old version of python? 3.7 will be EOL in a few months.

👍 will update

.github/workflows/ci.yml

mindaugasrukas · 2023-01-31T21:58:16Z

.github/workflows/ci.yml

@@ -0,0 +1,43 @@
+name: Lint and Test Charts
+
+on: push


I prefer running those on PRs instead.
Linting on push requires additional management, which we don't have right now.

mindaugasrukas · 2023-01-31T22:00:47Z

.github/workflows/ci.yml

+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v2


Let's use the v3 version.

mindaugasrukas · 2023-01-31T22:02:06Z

.github/workflows/ci.yml

+          fetch-depth: "0"
+
+      - name: Set up Helm
+        uses: azure/setup-helm@v1


The latest version is v3. Any reason for using v1?

mindaugasrukas · 2023-01-31T22:08:41Z

.github/workflows/ci.yml

+      - name: Set up Helm
+        uses: azure/setup-helm@v1
+        with:
+          version: v3.8.1


This is a year-old version. Do you have any reason not to use the latest?

mindaugasrukas · 2023-01-31T22:12:41Z

.github/workflows/ci.yml

+      - name: Set up chart-testing
+        uses: helm/chart-testing-action@v2.2.1
+        with:
+          version: v3.5.1


These are also old versions, any reason for not using the latest?

mindaugasrukas · 2023-01-31T22:13:52Z

ci/kind-test-values.yaml

@@ -0,0 +1,8 @@
+elasticsearch:


Where is this file used?

From https://github.com/helm/chart-testing/blob/main/doc/ct_install.md

Charts may have multiple custom values files matching the glob pattern '*-values.yaml' in a directory named 'ci' in the root of the chart's directory. The chart is installed and tested for each of these files. If no custom values file is present, the chart is installed and tested with defaults.

mindaugasrukas · 2023-01-31T22:14:31Z

ct.yml

@@ -0,0 +1,5 @@
+chart-repos:


The same here. Where is this file used?

underrun · 2023-04-25T15:30:48Z

I'd love to land this - but i'm inclined to skip the install step if it's not completing unless/until the deadlock gets sorted out

edmondop · 2023-04-25T15:48:07Z

Sure, I put it on hold because I was trying to get an hint on why this would not work on a kind cluster.

In the meanwhile I have realised that GitHub has added large runners, so I would give a shot using those, what do you think? Can anyone set up a runner group ? https://docs.github.com/en/actions/using-github-hosted-runners/using-larger-runners

.github/workflows/ci.yml

ct.yml

robholland · 2024-06-14T08:24:18Z

To make some iterative progress here, can you please re-base and get lint-only running? Then we can work on install checks after.

robholland · 2024-06-14T08:24:36Z

I will work with you to ensure this gets merged.

robholland

Re-base on current code, just lint for now.

edmondop · 2024-06-17T13:08:32Z

Re-base on current code, just lint for now.

Thanks, on it. Should be able to get it ready by eod

edmondop · 2024-06-17T16:41:55Z

Re-base on current code, just lint for now.

Thanks, on it. Should be able to get it ready by eod

@robholland should be good. I left the values for testing and removed the testing from the CI/CD pipeline, wdyt?

robholland

Please add a ci/ values file that tests cassandra + ElasticSearch combination (we mention that as our "default" in the README).

robholland · 2024-06-17T16:58:32Z

charts/temporal/templates/server-deployment.yaml

@@ -1,6 +1,6 @@
 {{- if $.Values.server.enabled }}
 {{- range $service := (list "frontend" "history" "matching" "worker") }}
-{{ $serviceValues := index $.Values.server $service }}
+{{- $serviceValues := index $.Values.server $service }}


The dash has actually recently been removed because it throws a warning in helm lint ;) #284 Did you add this back due to ct lint ?

No it might be result of a bad rebase:)

robholland · 2024-06-17T16:58:43Z

charts/temporal/templates/server-service.yaml

@@ -31,7 +31,7 @@ spec:

 ---
 {{- range $service := (list "frontend" "matching" "history" "worker") }}
-{{ $serviceValues := index $.Values.server $service }}
+{{- $serviceValues := index $.Values.server $service }}


Same as above.

robholland · 2024-06-17T16:59:27Z

charts/temporal/values.yaml

@@ -179,7 +179,7 @@ server:
          #   tx_isolation: 'READ-COMMITTED'
  frontend:
    service:
-      annotations: {} # Evaluated as template
+      annotations: {}  # Evaluated as template


Please revert this change.

robholland · 2024-06-17T17:00:14Z

.github/workflows/ci.yml

+      - name: Run chart-testing (lint)
+        run: ct lint --charts ./ --config=ct.yml
+
+      - name: Create kind cluster


We don't need the cluster for now as we're not testing install.

Do you suggest we add elasticsearch + cassandra but not run the tests in CI? Also I discovered I had to upgrade the action and now it doesn't work anymore :(

This is the last build with 2.4.0 that now fails because Google deprecated some APIs
https://github.com/edmondop/helm-charts/actions/runs/9551467429/job/26325831512

using the new version fails for other reasons , see https://github.com/edmondop/helm-charts/actions/runs/9551593568/job/26326271659#step:6:21. Maybe they changed the expected directory struxture, will investigate. If you are on the Slack channel you can ping me there so we can close this

Did ping on slack but haven't heard back. I suggested adding a new values file for the cassandra/ES case, but now that I think about it, maybe the values files doesn't matter for lint? And no, we shouldn't do the install tests yet, so don't need the Kind cluster.

edmondop requested review from a team as code owners January 22, 2023 01:39

edmondop force-pushed the issue-343 branch from 5a11a1d to c1c2274 Compare January 22, 2023 01:48

edmondop changed the title ~~Adding Linting and Testing on all committs~~ WIP: Adding Linting and Testing on all committs Jan 23, 2023

underrun reviewed Jan 26, 2023

View reviewed changes

mindaugasrukas reviewed Jan 31, 2023

View reviewed changes

nvtkaszpir suggested changes May 4, 2023

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

ct.yml Outdated Show resolved Hide resolved

edmondop force-pushed the issue-343 branch 2 times, most recently from 59ece92 to 2ab8cef Compare May 4, 2023 17:54

edmondop force-pushed the issue-343 branch from 2ab8cef to 55245a4 Compare August 1, 2023 23:38

robholland requested changes Jun 14, 2024

View reviewed changes

robholland added the needs revision Team has requested some changes label Jun 14, 2024

edmondop added 13 commits June 17, 2024 12:27

Adding ci yml

d35c7b0

Fixing --all slot for subfolders

98ac419

Added back build and install

a19e568

Fixing install syntax

97e1239

Including values with reduced resources for CI/CD

762256f

Including values with reduced cluster size

424fa4d

Cassandra cluster should have one replica

5a7f254

Increasing timeout

101475d

Is it just a timeout thing?

214acf9

Is it inotify?

6ff4b6c

Sudo

3cc03df

Sudo

eb603fc

Removed ignored inotify changes

e57b845

edmondop added 11 commits June 17, 2024 12:27

Up versions

6a27697

Python version as well

f8911ad

Update lint settings

dce77f4

Fixing validation

e8ff2c8

Addressing PR suggestion

55d4133

Addressing linting issues after rebasing

4661b43

Trying to test the chart here and there

6203acf

Trying to test the chart here and there

a4655b0

Adding back chart repo deps

2fb5215

Adding second part to test

de27c4a

Maybe up will work

9547e99

edmondop force-pushed the issue-343 branch from 40e40cb to 9547e99 Compare June 17, 2024 16:30

edmondop requested a review from a team as a code owner June 17, 2024 16:30

edmondop added 3 commits June 17, 2024 12:35

Removing wrong piece of configuration

89a4c7c

Removing testing fro Github Workflows

11a6588

Removing newline

d20520e

Update of chart testing action to reflect googleapis deprecation

12d0244

robholland requested changes Jun 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Adding Linting and Testing on all committs #352

WIP: Adding Linting and Testing on all committs #352

edmondop commented Jan 22, 2023 •

edited

Loading

underrun Jan 26, 2023

edmondop Jan 26, 2023

mindaugasrukas Jan 31, 2023

mindaugasrukas Jan 31, 2023

mindaugasrukas Jan 31, 2023

mindaugasrukas Jan 31, 2023

mindaugasrukas Jan 31, 2023

mindaugasrukas Jan 31, 2023

edmondop Jan 31, 2023

mindaugasrukas Jan 31, 2023

underrun commented Apr 25, 2023

edmondop commented Apr 25, 2023

robholland commented Jun 14, 2024

robholland commented Jun 14, 2024

robholland left a comment

edmondop commented Jun 17, 2024

edmondop commented Jun 17, 2024

robholland left a comment

robholland Jun 17, 2024

edmondop Jun 17, 2024

robholland Jun 17, 2024

robholland Jun 17, 2024

robholland Jun 17, 2024

edmondop Jun 17, 2024

robholland Jun 24, 2024

WIP: Adding Linting and Testing on all committs #352

Are you sure you want to change the base?

WIP: Adding Linting and Testing on all committs #352

Conversation

edmondop commented Jan 22, 2023 • edited Loading

What was changed

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

underrun commented Apr 25, 2023

edmondop commented Apr 25, 2023

robholland commented Jun 14, 2024

robholland commented Jun 14, 2024

robholland left a comment

Choose a reason for hiding this comment

edmondop commented Jun 17, 2024

edmondop commented Jun 17, 2024

robholland 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

Choose a reason for hiding this comment

edmondop commented Jan 22, 2023 •

edited

Loading