feat: add config header#58886
Conversation
|
Please note that I asked on NC Talk how I could test this code before creating the PR and did not get an answer, I would love to test it before we move forward, I solidify my claim that this works as expected. |
provokateurin
left a comment
There was a problem hiding this comment.
Nice addition! I think there are some small things to improve, but overall it looks very good already. I will let the CI run once these things have been adjusted.
|
Do I need to do anything more or regularly update the branch until it is merged or is my side of things done? |
|
You need to update the tests to account for your changes. You can see the failures here: https://github.com/nextcloud/server/actions/runs/23019402943/job/66860023752?pr=58886. You can run them locally using Please squash all your commits and make sure they follow https://www.conventionalcommits.org/en/v1.0.0/. |
provokateurin
left a comment
There was a problem hiding this comment.
Tests need to be updated.
|
I just tried to run a rebase, because I originally started working on this a half year ago there are ~5600 commits in the squash commits including conflicts that I need to resolve, it may be better to close this PR and reopen a new branch based on the current HEAD and the comments that were here, unless there is some better tool to do this. |
|
That's not necessary. If you want I can rebase your PR for you. I could also adjusts the tests, but maybe you'd like to do that yourself? Just let me know what you need from us :) |
|
I'm happy to try on the one hand, on the other it seems to be more work than just creating a new branch and a new PR... What I tried to do was If there is a better/faster way to do this I'm happy to learn but it seems like a waste of time to me to start resolving merge conflicts in the past half year of commits that were for sure already resolved/were never an issue to begin with. |
|
A |
92938b7 to
11b8538
Compare
|
That doesn't look like it squashed everything unless I'm very much mistaken. /Edit - I ran: |
|
Yes sorry, I also realized my instructions might not match your setup. |
|
Can you adjust the tests now so they pass? |
|
When you say adjust the tests what exactly do you mean? |
11b8538 to
373da49
Compare
You need to add your new header to the text in \Test\ConfigTest::TESTCONTENT You can run the test locally by running If you need more help, join us in our community dev chat |
|
Cypress and performance won't run on your fork, so nothing to do there. Can you squash your commits into a single one and sign the commit? easiest way is: |
Signed-off-by: E.S. Rosenberg a.k.a. Keeper of the Keys <es-github@rosenberg.org.il>
c0a5cd9 to
16ebbee
Compare
|
OOF, looks like I should have rebased first based on the amount of files changed. |
Summary
config.phpis re-rendered by nextcloud regularly removing comments and rendering constants into their values, nextcloud supports using multiple config files and renders their content into the main config.php without modifying the external files.This PR adds a header to
config.phpwarning the admin/user of this and suggesting the use of the multiple config files mechanism.TODO
config.phpin the order that they were included will probably help admins troubleshoot if a config variable is doubled up by mistake.Checklist
Tests (unit, integration, api and/or acceptance) are includedScreenshots before/after for front-end changesBackports requested where applicable (ex: critical bugfixes)3. to review, feature component)stable32)AI (if applicable)
The content of this PR was partly or fully generated using AI