Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve handling of config file #480

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wrenix
Copy link
Contributor

@wrenix wrenix commented Nov 20, 2023

Split of #478

Move all the configfiles for nginx, nextcloud, htaccess out of the kubernetes-resources as single file beside the helmchart and load them with helm function into the kubernetes-resources.

benefits:

  • config files are readible (still support of golang/sring/helm-templates)
  • logic of which defaultConfigs is loaded is generic (is an range over the map)

@jessebot
Copy link
Collaborator

jessebot commented Nov 21, 2023

Please detail in the description of this PR what you're doing and why to make it easier to review. Ideally, please use the PR template.

@wrenix
Copy link
Contributor Author

wrenix commented Nov 22, 2023

Done

@wrenix
Copy link
Contributor Author

wrenix commented Nov 27, 2023

@jessebot do you like to review?

@wrenix
Copy link
Contributor Author

wrenix commented Nov 30, 2023

rebased after #393 was merged

ci lint says:

Update Complete. ⎈Happy Helming!⎈
Saving 3 charts
Downloading postgresql from repo oci://registry-1.docker.io/bitnamicharts
Downloading mariadb from repo oci://registry-1.docker.io/bitnamicharts
Downloading redis from repo oci://registry-1.docker.io/bitnamicharts
Deleting outdated charts
Pulled: registry-1.docker.io/bitnamicharts/postgresql:12.12.10
Digest: sha256:179d5c6d017298bb9ab868321bcacb1a9efe5eb8224902f89f8693c69a72e428
Pulled: registry-1.docker.io/bitnamicharts/mariadb:12.2.9
Digest: sha256:834d78c385839bac4a7ec8cd0d25adea6b9a3324ef8c7e284e59d9e33c1e96b6
Pulled: registry-1.docker.io/bitnamicharts/redis:17.13.2
Digest: sha256:b7892f0842f1758bb35c61aaccdbb2ca30a7ff234477a2b270de31db8c0920e0
Linting chart "nextcloud => (version: \"4.5.3\", path: \"charts/nextcloud\")"
Checking chart "nextcloud => (version: \"4.5.3\", path: \"charts/nextcloud\")" for a version bump...
Old chart version: 4.4.0
New chart version: 4.5.3
Chart version ok.
Validating /home/wrenix/src/github.com/nextcloud/helm/charts/nextcloud/Chart.yaml...
Validation success! 👍
Validating maintainers...

Linting chart with values file "charts/nextcloud/ci/ct-all-enabled-values.yaml"...

==> Linting charts/nextcloud

1 chart(s) linted, 0 chart(s) failed

Linting chart with values file "charts/nextcloud/ci/ct-empty-values.yaml"...

==> Linting charts/nextcloud

1 chart(s) linted, 0 chart(s) failed

Linting chart with values file "charts/nextcloud/ci/ct-specials-values.yaml"...

==> Linting charts/nextcloud

1 chart(s) linted, 0 chart(s) failed

------------------------------------------------------------------------------------------------------------------------
 ✔︎ nextcloud => (version: "4.5.3", path: "charts/nextcloud")
------------------------------------------------------------------------------------------------------------------------
All charts linted successfully

@wrenix
Copy link
Contributor Author

wrenix commented Dec 8, 2023

rebased and version bump again

@jessebot
Copy link
Collaborator

It looks like there's an error after the nginx default config PR was merged.

https://github.com/nextcloud/helm/actions/runs/7144375839/job/19639651937?pr=480#step:9:219

nginx:
enabled: true
config:
custom: |-
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is just my test-value here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe that is better

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved the new workflow run. I wish that ran automatically still :(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain further the ci directory you are adding? I don't think I understand what this is doing.

Copy link
Contributor Author

@wrenix wrenix Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ci/*-values.yaml is used by chart-testing. it try to lint / install with every of that files (I add three -values.yaml files, so an lint or install tries this three setups).

your chart-testing github action pipeline try an install in a kubernetes clusters.
So it checks, if all helm generated resources also comes up (like helm --wait).
But with my current example with an customize nginx config (nginx.config.custom).
is put correct in the configmap and to the pod, but the nginx does not reach the state running (because the nginx config is invalid with my test values).

I just use the ct lint command on developing, so i have not seen, that this chart works (and the part is correct put together).


https://github.com/helm/chart-testing/blob/main/doc/ct_lint.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 linted for each of these files. If no custom values file is present, the chart is linted with defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i remove the ct-values for #523 - to make this PR smaller

@wrenix wrenix force-pushed the fix/file-config branch 3 times, most recently from 9181f14 to 5660b62 Compare December 19, 2023 21:28
@wrenix
Copy link
Contributor Author

wrenix commented Dec 19, 2023

rebased again and set an valid nginx.config.custom in charts/nextcloud/ci/ct-specials-values.yaml.

@wrenix
Copy link
Contributor Author

wrenix commented Dec 25, 2023

rebased and version dump again

@wrenix wrenix force-pushed the fix/file-config branch 2 times, most recently from 9361dc8 to 96ae2ab Compare February 4, 2024 17:23
@wrenix
Copy link
Contributor Author

wrenix commented Feb 4, 2024

rebased (after #520) and version dump again

@wrenix wrenix force-pushed the fix/file-config branch 2 times, most recently from 48a7885 to 5a7cb38 Compare February 5, 2024 07:44
@wrenix
Copy link
Contributor Author

wrenix commented Feb 5, 2024

huhu @provokateurin do you like to take a look here too?

@wrenix wrenix force-pushed the fix/file-config branch 3 times, most recently from c592d5a to 2714d88 Compare February 5, 2024 11:39
@wrenix wrenix mentioned this pull request Feb 5, 2024
3 tasks
@wrenix
Copy link
Contributor Author

wrenix commented Feb 5, 2024

This is the last for nd importend PR from my big bunch ;)
Thanks so much till here.

@provokateurin
Copy link
Member

I'll have a look soon

Signed-off-by: WrenIX <dev.github@wrenix.eu>
@wrenix wrenix reopened this Mar 31, 2024
@wrenix
Copy link
Contributor Author

wrenix commented Mar 31, 2024

I do not know, why it / i close this

@wrenix
Copy link
Contributor Author

wrenix commented Apr 19, 2024

Any update?

I found a not good default value for nginx, which i like to improve (and then i good a merge conflict)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants