search

project-zot / helm-charts

7 stars 19 forks source link

Persistence config ingress #2

Closed sto closed 1 year ago

sto commented 1 year ago

Add support to the chart to use persistence, configuration managed as secrets and to deploy an ingress definition for zot.

sto commented 1 year ago

I don't know if the changes are interesting for the official chart, but as I'm using them on my own deployment I created this PR just in case you find any of them useful, feel free to ignore the PR or use only a part of it.

rchincha commented 1 year ago

@sto Thanks for posting this PR. Apologies that we have not looked at this until now. We were not expecting any contributions here.

@Andreea-Lupu, can you pls review this? Also this: https://github.com/project-zot/zot/discussions/1386

rchincha commented 1 year ago

@Andreea-Lupu Can we add a ci pipeline test using for this repo, so contributions/PRs are validated? We can use kind for that.

https://kind.sigs.k8s.io/docs/user/quick-start/

rchincha commented 1 year ago

@sto, can you pls squash your commits?

sto commented 1 year ago

@sto, can you pls squash your commits?

@rchincha, rebased and squashed, if you need any additional changes please let me now.

rchincha commented 1 year ago

@sto we are adding some ci/cd tests so we can safely accept PRs like yours after vetting.

rchincha commented 1 year ago

https://github.com/project-zot/helm-charts/pull/5 @sto fyi. We plan to merge this first.

Pls add a test to validate that "ingress" is working correctly.

rchincha commented 1 year ago

@sto pls. rebase and re-push this PR - would at least like to get the DCO checks passed. If you are unable to add a test, we can do that also.

sto commented 1 year ago

@sto pls. rebase and re-push this PR - would at least like to get the DCO checks passed. If you are unable to add a test, we can do that also.

I rebased and repushed, haven't looked at the testing part and I'll be unable to look at it until the weekend.

I'm unsure about what has to be done, but if there are no news on Saturday I'll look into it.

rchincha commented 1 year ago

@sto

For the DCO check failure: https://github.com/project-zot/helm-charts/actions/runs/4919517472/jobs/8796492154?pr=2

Missing sign-off(s):

32c5ecaded092374cdcee13fc40a736eacbed9db
    no sign-off found

Also pls sign (GPG etc) your commits.

There is also a helm lint failure https://github.com/project-zot/helm-charts/actions/runs/4919517474/jobs/8796492330?pr=2

Error: 1 chart(s) linted, 1 chart(s) failed ==> Linting ./charts/zot [INFO] Chart.yaml: icon is recommended Error: values.yaml: unable to parse YAML: error converting YAML to JSON: yaml: line 74: did not find expected key Error: templates/: cannot load values.yaml: error converting YAML to JSON: yaml: line 74: did not find expected key Error: : unable to load chart cannot load values.yaml: error converting YAML to JSON: yaml: line 74: did not find expected key

rchincha commented 1 year ago

@sto thanks for working patiently with us on this. Hope you understand this is mostly to protect the investment you are making with this PR.

git commit --amend --no-edit -s -S to fix the DCO failure. The other failure is on us, will take care of that.

sto commented 1 year ago

@sto thanks for working patiently with us on this. Hope you understand this is mostly to protect the investment you are making with this PR.

git commit --amend --no-edit -s -S to fix the DCO failure. The other failure is on us, will take care of that.

Done.

Andreea-Lupu commented 1 year ago

Hi, @sto. Can you please increment the chart version("version" field from Chart.yaml file) and add a space before 'authHeader' at this line https://github.com/project-zot/helm-charts/pull/2/files#diff-3e090fa767e19b6bf71a738b87e9d9e2af22493b398c58c8d4c35de76e1bb2f9R98? These changes will fix the test which fails.

Also, I tried to test your code with mountConfig: true , but when I tried to install the chart I got this error: ` Error: INSTALLATION FAILED: 1 error occurred:

sto commented 1 year ago

Done, the issue with the configmap was that I was missing a quote on the templates.

The changes made are as follows (the diff is from my latest changes before squashing them):

diff --git a/charts/zot/Chart.yaml b/charts/zot/Chart.yaml
index 7cebb6e..be53a98 100644
--- a/charts/zot/Chart.yaml
+++ b/charts/zot/Chart.yaml
@@ -3,4 +3,4 @@ appVersion: v2.0.0-rc4
 description: A Helm chart for Kubernetes
 name: zot
 type: application
-version: 0.1.21
+version: 0.1.22
diff --git a/charts/zot/templates/configmap.yaml b/charts/zot/templates/configmap.yaml
index ba2ac2f..a6a29c3 100644
--- a/charts/zot/templates/configmap.yaml
+++ b/charts/zot/templates/configmap.yaml
@@ -5,6 +5,6 @@ metadata:
   name: {{ .Release.Name }}-config
 data:
 {{- range $key, $val := .Values.configFiles }}
-  {{ $key }}: {{ $val }}
+  {{ $key }}: {{ $val | quote }}
 {{- end }}
 {{- end }}
diff --git a/charts/zot/values.yaml b/charts/zot/values.yaml
index 778395e..f95f1de 100644
--- a/charts/zot/values.yaml
+++ b/charts/zot/values.yaml
@@ -95,7 +95,7 @@ secretFiles:
 # in base64. It is needed when `htpasswd` authentication is enabled and the
 # default access does not provide read permission
 # The example value is from running `echo -n "foo:var" | base64`
-#authHeader: "Zm9vOmJhcg=="
+# authHeader: "Zm9vOmJhcg=="

 # If persistence is 'true' the service uses a persistentVolumeClaim to mount a
 # volume for zot on '/var/lib/registry'; by default the pvc used is named
rchincha commented 1 year ago

@sto @liangyuanpeng this PR is merged, also shows up at https://artifacthub.io/packages/helm/zot/zot Pls try and let us know. Thanks @sto