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

Warn if config folder misses original image configs or files differ #2120

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

flortsch
Copy link
Contributor

@flortsch flortsch commented Dec 13, 2023

Implements a check in the Docker entrypoint that compares the special config files of this Docker image with the config files of the user folder. It prints a warning if those config files are missing or they differ from the original Docker image config files. This is a first step, as suggested by @joshtrichards in #1533. In the long run, these files should be treated as special config files not to be messed with by the user, and they should be copied over into the user config folder on every update.

Fixes #1533

docker-entrypoint.sh Outdated Show resolved Hide resolved
@joshtrichards
Copy link
Member

Thanks @flortsch!

I'm thinking we should include a new paragraph in the README to refer people to with questions about what is going on here / help them know what they should do. Based on experience with the setup warnings in Nextcloud's Admin Overview, many people pay a lot of attention to these things and get frustrated if they're not able to remedy warnings (understandable reaction + remedying this is the goal!).

Ideally we include that in this PR, but it can also be in an immediately following one if you'd prefer to keep separate.

@flortsch
Copy link
Contributor Author

Hey @joshtrichards! Sound good. I can extend the PR with a section in the readme and a link to it in the console output. Since you mentioned warnings in the Nextcloud admin overview: is it possible to generate such UI warnings in the entrypoint script? I think that warnings in the UI would be a nice addition to the console output which can often be overlooked.

@joshtrichards joshtrichards added feature: auto config (environment variables) Auto configuring via environment variables feature: upgrading Upgrading an existing deployment to a new image/NC version labels Dec 29, 2023
@flortsch flortsch force-pushed the warn-user-config branch 5 times, most recently from 26cb406 to 3f9d895 Compare January 12, 2024 01:01
@flortsch
Copy link
Contributor Author

@joshtrichards Please check if the text and link is ok. I updated the README and added a link in the docker-entrypoint echo statement. Also, I inlined your last text feedback.

@J0WI J0WI added the docs label Jan 16, 2024
docker-entrypoint.sh Outdated Show resolved Hide resolved
@flortsch flortsch force-pushed the warn-user-config branch 2 times, most recently from fafa06d to c8c117b Compare February 11, 2024 13:19
@joshtrichards
Copy link
Member

Since you mentioned warnings in the Nextcloud admin overview: is it possible to generate such UI warnings in the entrypoint script? I think that warnings in the UI would be a nice addition to the console output which can often be overlooked.

Not currently.

Copy link
Member

@joshtrichards joshtrichards left a comment

Choose a reason for hiding this comment

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

Finally got a chance to test this a bit. Made some proposed adjustments. Let me know what you think.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docker-entrypoint.sh Outdated Show resolved Hide resolved
@flortsch flortsch force-pushed the warn-user-config branch 2 times, most recently from f9e30e0 to 089413e Compare March 18, 2024 20:15
@flortsch
Copy link
Contributor Author

flortsch commented Mar 18, 2024

Hey @joshtrichards! I included your suggestions. Only fixed the formatting and the uninitialized variable access. Tested locally with docker-compose (fresh start + changes in existing container/volume). Missing or changed image-specific files trigger the warning & file listing. Custom config files don't trigger anything. Commits squashed and rebased on latest master.

@joshtrichards
Copy link
Member

@J0WI What do you think about this?

I think a warning when the configs are out-of-date is a good step forward without introducing a breaking change. It's also something easy to check (or request the reporter check) in the container logs whenever a new bug comes in is unknowingly bogus due to out-of-date config files.

The other option is we take the full leap: documenting all the image config/ files (names) as being "special" and exclusive to the image. Then we update them automatically whenever needed. That'll cause it's own problems (at least in the short-term) IMO.

We can always take the bigger leap later on after we add the warning. So I think this is a good compromise.

docker-entrypoint.sh Outdated Show resolved Hide resolved
Co-authored-by: Josh <[email protected]>
Signed-off-by: Florian Latifi <[email protected]>
@flortsch
Copy link
Contributor Author

flortsch commented Jun 21, 2024

@joshtrichards I reverted the changes and simplified the code back to just print a one-line warning for every config file that differs. What do you think?

Copy link
Contributor

@J0WI J0WI left a comment

Choose a reason for hiding this comment

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

This looks much cleaner, thanks!

@J0WI J0WI merged commit 5c58b2a into nextcloud:master Jun 25, 2024
17 of 21 checks passed
@joshtrichards
Copy link
Member

Thanks, @flortsch for getting this started and hanging in there! :-)

@joshtrichards joshtrichards added this to the Nextcloud 29.0.3 milestone Jun 26, 2024
@joshtrichards
Copy link
Member

First sighting in the wild: https://help.nextcloud.com/t/question-on-some-warnings-on-outdated-files-since-update-to-29-0-3/196297

Maybe the docs link should get added back in :-)

@flortsch
Copy link
Contributor Author

flortsch commented Jun 27, 2024

I myself got this warning today after updating to 28.0.7 🤣 Will double-check my configs, but it seems the check fulfills its purpose 😄 If the confusion gets to big I can provide a PR with the link to the README back in place.

@joshtrichards
Copy link
Member

Warning: /var/www/html/config/autoconfig.php differs from the latest version of this image at /usr/src/nextcloud/config/autoconfig.php

I guess this one should be excluded from the check. It's removed automatically (by the Nextcloud installer) after use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review docs enhancement feature: auto config (Docker secrets) feature: auto config (environment variables) Auto configuring via environment variables feature: upgrading Upgrading an existing deployment to a new image/NC version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Special config files like reverse-proxy.config.php are not updated in existing installations when upgrading
3 participants