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 files #480

Merged
merged 7 commits into from
Jul 24, 2024

Conversation

wrenix
Copy link
Collaborator

@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 helm chart and load them with helm function into the kubernetes-resources.

benefits:

  • config files are readable (still support of golang/sring/helm-templates)
  • logic in which defaultConfigs is loaded is generic (it is a 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
Collaborator Author

wrenix commented Nov 22, 2023

Done

@wrenix
Copy link
Collaborator Author

wrenix commented Nov 27, 2023

@jessebot do you like to review?

@wrenix wrenix mentioned this pull request Nov 27, 2023
3 tasks
@wrenix
Copy link
Collaborator 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 wrenix force-pushed the fix/file-config branch 5 times, most recently from 09f5f70 to d03fc12 Compare December 2, 2023 10:40
@wrenix
Copy link
Collaborator 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

@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
Collaborator 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
Collaborator 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
Collaborator Author

wrenix commented Feb 4, 2024

rebased (after #520) and version dump again

@wrenix wrenix force-pushed the fix/file-config branch from 57ed963 to c0277e8 Compare June 14, 2024 21:19
@wrenix wrenix force-pushed the fix/file-config branch 2 times, most recently from 879b0f6 to d232877 Compare June 14, 2024 21:22
@provokateurin
Copy link
Member

Hi and sorry for the super long delay. In principle I agree with the changes, I just find it hard to review as it is not so easy to verify nothing in the config file was actually changed. I'll try to give it a review soon.

@wrenix wrenix force-pushed the fix/file-config branch from d232877 to cfc85ac Compare June 14, 2024 22:00
@jessebot
Copy link
Collaborator

merged main and kicking off tests again, this time with new database tests :)

@jessebot
Copy link
Collaborator

jessebot commented Jul 22, 2024

OK, now that I've finally spent some time looking at this, my first new request is that you rename the .gotmpl file extensions to be .tpl as that seems to be standard. I'm gonna point my Argo CD ApplicationSet at this branch and if everything is working, we'll call this good to go and merge it. If anything breaks, I may kindly ping you @wrenix 🙏

@jessebot jessebot self-requested a review July 22, 2024 11:57
jessebot added a commit to small-hack/argocd-apps that referenced this pull request Jul 22, 2024
@jessebot
Copy link
Collaborator

jessebot commented Jul 22, 2024

I've tested this and it looks good to me :) Please still update the gotmpl file extensions to be tpl though, just for consistency.

@provokateurin
Copy link
Member

Nice stuff, let's get this merged finally :)

@jessebot
Copy link
Collaborator

@wrenix when you get a chance, could you please update the file extensions to tpl for the config files? Then we can get this merged :)

@jessebot
Copy link
Collaborator

jessebot commented Jul 24, 2024

@provokateurin, since wrenix is a little busy right now, how about after the tests run (assuming they are all valid), we merge this, and then in #464, I can refactor it to use the new format that wrenix has introduced here, and then also change the file extensions there, so all the tpl files are consistent?

Edit: I would be squashing and merging this, to prevent history from being weird :)

@provokateurin
Copy link
Member

I think the file extensions should be fixed in this PR, but other than that it sounds good.

@jessebot
Copy link
Collaborator

I think the file extensions should be fixed in this PR, but other than that it sounds good.

Turns out since maintainers were given access to the PR, I was actually able to clone wrenix's fork and just update the extensions locally and push them up 👍

@jessebot jessebot changed the title fix: improve handling of config file fix: improve handling of config files Jul 24, 2024
@jessebot jessebot merged commit bf6cc4a into nextcloud:main Jul 24, 2024
8 checks passed
@wrenix wrenix deleted the fix/file-config branch August 11, 2024 19:11
@wrenix
Copy link
Collaborator Author

wrenix commented Aug 11, 2024

oh sorry, i have overseen that review. And it is merged, now.
Nice and thank you for patching (the extension).

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.

3 participants