-
Notifications
You must be signed in to change notification settings - Fork 271
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
Conversation
ff0c6bc
to
e5c7e61
Compare
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. |
Done |
@jessebot do you like to review? |
1db6808
to
c74a666
Compare
rebased after #393 was merged ci lint says:
|
09f5f70
to
d03fc12
Compare
d03fc12
to
c966cc9
Compare
rebased and version bump again |
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 |
9181f14
to
5660b62
Compare
rebased again and set an valid |
5660b62
to
a6ca7bd
Compare
rebased and version dump again |
9361dc8
to
96ae2ab
Compare
rebased (after #520) and version dump again |
96ae2ab
to
36f875d
Compare
879b0f6
to
d232877
Compare
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. |
Signed-off-by: WrenIX <[email protected]>
Signed-off-by: Jesse Hitch <[email protected]>
merged main and kicking off tests again, this time with new database tests :) |
Signed-off-by: Jesse Hitch <[email protected]>
OK, now that I've finally spent some time looking at this, my first new request is that you rename the |
I've tested this and it looks good to me :) Please still update the gotmpl file extensions to be |
Nice stuff, let's get this merged finally :) |
@wrenix when you get a chance, could you please update the file extensions to |
Signed-off-by: Jesse Hitch <[email protected]>
Signed-off-by: Jesse Hitch <[email protected]>
@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 Edit: I would be squashing and merging this, to prevent history from being weird :) |
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 👍 |
Signed-off-by: jessebot <[email protected]>
22afec9
to
eba90dd
Compare
oh sorry, i have overseen that review. And it is merged, now. |
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:
defaultConfigs
is loaded is generic (it is arange
over the map)