temporalio / helm-charts

Temporal Helm charts
MIT License
312 stars 339 forks source link

WIP: Adding Linting and Testing on all committs #352

Closed edmondop closed 3 months ago

edmondop commented 1 year ago

What was changed

Addresses #343

Getting the build to pass seems to be blocked by https://github.com/temporalio/helm-charts/issues/182 but I don't think disabling hooks is appropriate:

https://github.com/temporalio/helm-charts/compare/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 commented 1 year ago

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 commented 1 year ago

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

robholland commented 4 months ago

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 commented 4 months ago

I will work with you to ensure this gets merged.

edmondop commented 4 months ago

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

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

edmondop commented 4 months ago

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 commented 3 months ago

Fixed by https://github.com/temporalio/helm-charts/commit/6f9c93dc5b2f767f98e9c917911b75f78cc1a3aa