apache / polaris

Apache Polaris, the interoperable, open source catalog for Apache Iceberg
https://polaris.apache.org/
Apache License 2.0
1.17k stars 130 forks source link

Add support of setting revisionHistoryLimit #386

Closed MonkeyCanCode closed 2 weeks ago

MonkeyCanCode commented 1 month ago

Description

The current Helm chart doesn’t allow customization of the revisionHistoryLimit parameter, which is crucial for controlling the number of ReplicaSets retained for K8S deployment. This PR introduces the ability to set revisionHistoryLimit in the values file, defaulting to the standard value if not specified.

Also, I cleanup one comment in ingress.yaml which is not needed.

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

When revisionHistoryLimit is not set:

xxx@DESKTOP:~/k8s/polaris/helm/polaris(add_revisionHistoryLimit)$ cat myvalues.yaml | grep revision
revisionHistoryLimit: ~
xxx@DESKTOP:~/k8s/polaris/helm/polaris(add_revisionHistoryLimit)$ helm template polaris -f myvalues.yaml . | grep revision
walk.go:74: found symbolic link in path: /home/yong/k8s/polaris/helm/polaris/LICENSE resolves to /home/yong/k8s/polaris/LICENSE. Contents of linked file included and used

When revisionHistoryLimit is set to 0:

xxx@DESKTOP:~/k8s/polaris/helm/polaris(add_revisionHistoryLimit)$ cat myvalues.yaml | grep revision
revisionHistoryLimit: 0
xxx@DESKTOP:~/k8s/polaris/helm/polaris(add_revisionHistoryLimit)$ helm template polaris -f myvalues.yaml . | grep revision
walk.go:74: found symbolic link in path: /home/yong/k8s/polaris/helm/polaris/LICENSE resolves to /home/yong/k8s/polaris/LICENSE. Contents of linked file included and used
  revisionHistoryLimit: 0

When revisionHistoryLimit is set to 1:

xxx@DESKTOP:~/k8s/polaris/helm/polaris(add_revisionHistoryLimit)$ cat myvalues.yaml | grep revision
revisionHistoryLimit: 1
xxx@DESKTOP:~/k8s/polaris/helm/polaris(add_revisionHistoryLimit)$ helm template polaris -f myvalues.yaml . | grep revision
walk.go:74: found symbolic link in path: /home/yong/k8s/polaris/helm/polaris/LICENSE resolves to /home/yong/k8s/polaris/LICENSE. Contents of linked file included and used
  revisionHistoryLimit: 1

Checklist:

Please delete options that are not relevant.

MonkeyCanCode commented 3 weeks ago

@adutra new PR for the remaining manifest had being created in https://github.com/apache/polaris/pull/409. Please take a look when you get a chance and let me know if anything should be changed.

adutra commented 2 weeks ago

@MonkeyCanCode I merged #409 before this one, which was probably a mistake. And now we have a conflict on this PR. Could you resolve the conflict please? I'll merge as soon as CI passes.

MonkeyCanCode commented 2 weeks ago

@MonkeyCanCode I merged #409 before this one, which was probably a mistake. And now we have a conflict on this PR. Could you resolve the conflict please? I'll merge as soon as CI passes.

Checking

MonkeyCanCode commented 2 weeks ago

@adutra PR is fixed. What u did there is correct. The problem for the conflict is due to both MR making the same change to ingress.yaml (I fixed one thing there earlier for this PR...then one week later fix the same on the PR you just merged along with additional unit cases etc.). Here is the local test status:

yong@DESKTOP:~/k8s/polaris(add_revisionHistoryLimit)$ helm unittest helm/polaris/ 2>&1 | grep -v 'symbolic link'

### Chart [ polaris ] helm/polaris/

 PASS   helm/polaris/tests/configmap_test.yaml
 PASS   helm/polaris/tests/deployment_test.yaml
 PASS   helm/polaris/tests/hpa_test.yaml
 PASS   helm/polaris/tests/ingress_test.yaml
 PASS   helm/polaris/tests/job_test.yaml
 PASS   helm/polaris/tests/service_test.yaml
 PASS   helm/polaris/tests/serviceaccount_test.yaml

Charts:      1 passed, 1 total
Test Suites: 7 passed, 7 total
Tests:       156 passed, 156 total
Snapshot:    0 passed, 0 total
Time:        408.283818ms