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.

Sign up for GitHub

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

Jump to bottom

Updater doesn't delete some files automatically #18435

Open
tsteur opened this issue Dec 2, 2021 · 6 comments
Open

Updater doesn't delete some files automatically #18435

tsteur opened this issue Dec 2, 2021 · 6 comments
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github.
Milestone
5.4.0

Comments

@tsteur
Copy link
Member

tsteur commented Dec 2, 2021 •
edited

Seen eg in #18411 (comment) and #18415 #18418

At least some of these files (I'm thinking node_modules/iframe-resizer/.eslintrc and node_modules/ng-dialog/.eslintrc, and vendor/php-di/php-di/.gitstats.yml and vendor/php-di/php-di/.phpstorm.meta.php) we would have expected to be deleted automatically eg in https://github.com/matomo-org/matomo/blob/4.6.1/plugins/CoreUpdater/Model.php#L26-L36 Unless there's a different problem that these files shouldn't have been in the manifest file or so.

Also maybe we can check if we can delete some files in the Matomo root directory which we haven't done in the past. This can be tricky though as some files like a .htaccess we wouldn't want to delete. As part of this issue we investigate if we can delete some of these files automatically, or whether tests can help us make aware that we need to delete certain files manually if they are gone in the release (might be tricky as it's done in the build script I assume). If we can't delete them automatically, and can't test for it, then we need to make sure that when we make changes to the package script that we are made aware in any way to not forget to change this.

Maybe even a test could otherwise fetch the build script and parse for rm -rf calls etc.
@tsteur tsteur added the Bug For errors / faults / flaws / inconsistencies etc. label Dec 2, 2021
@tsteur tsteur added this to the 4.7.0 milestone Dec 2, 2021
@sgiehl
Copy link
Member

sgiehl commented Dec 3, 2021

@tsteur We are actually ignoring all files starting with . on update. Those file won't be deleted if they exist.
The reason for ignoring is actually the glob(*) we are using. It doesn't catch hidden files. This can actually be fixed and I will prepare a PR for it.

@sgiehl sgiehl mentioned this issue Dec 3, 2021
Merged
11 tasks
@sgiehl
Copy link
Member

sgiehl commented Dec 3, 2021

@tsteur regarding the other part of this issue. I wouldn't start trying to sync/remove other parts. Guess we had a good reason not to do that. To circumvent we have similar issues in the future we could create a github action, that automatically checks the changes in a PR. And if a file is deleted, that wouldn't be caught by the updater we could add a comment on the PR to check if an update script is provided to remove those files.
Guess it should be quite simple to do that. Just something like git log {BASESHA}..{PRSHA} --diff-filter=D --summary | grep "delete mode" and then go through the list and check if it is in certain directories or not...
Or would you prefer another approach?

@tsteur
Copy link
Member Author

tsteur commented Dec 5, 2021

Would that action run on the archive repository? As we might delete a file from root directory in the script that builds the release? And it would run in this repository?

could maybe also a regular test run the above command so it runs as part of the tests?

@sgiehl
Copy link
Member

sgiehl commented Dec 6, 2021

No, that action would run in this repository for all pull request.
I don't yet have a good idea how to write a test for that. We would actually need to build a release. Download the latest release, perform an update, and check if the file integrity check complains about anything. Imho that sounds a bit oversized for a normal test. It might possibly be something I could implement it #17594 and let a release fail if the integrity check complains 🤔

@tsteur
Copy link
Member Author

tsteur commented Dec 6, 2021

👍 OK. I was hoping we could just run git log {BASESHA}..{PRSHA} --diff-filter=D --summary | grep "delete mode" in a regular test but maybe something like that doesn't work

@justinvelluppillai justinvelluppillai modified the milestones: 4.7.0, 4.8.0 Jan 18, 2022
@tsteur tsteur modified the milestones: 4.8.0, 4.9.0 Feb 9, 2022
@atom-box
Copy link

atom-box commented Feb 9, 2022 •
edited

A user noticed this issue:
"The one thing I want to see improved? The ... file checks during the update. Every ... time the updater nags me about supposedly unexpected files that I'm supposed to manually delete before I can continue. each. .... Just. These are files that Matomo put in there himself during the last update, why are they unexpected? And if they suddenly bother you, why doesn't the updater delete the ... itself or at least offer me a button that I can use to do this conveniently so that I don't have to log in to the server myself? I updated Matomo two days ago and of course had to delete files manually again. Now I get another mail about an update to 4.7.1, and of course I have to delete a few other files manually. Get your build process on track or teach the updater not to be such a chump"

@tsteur tsteur added Critical Indicates the severity of an issue is very critical and the issue has a very high priority. and removed Critical Indicates the severity of an issue is very critical and the issue has a very high priority. labels Mar 2, 2022
@tsteur tsteur modified the milestones: 4.9.0, 4.10.0 Mar 2, 2022
@Findus23 Findus23 mentioned this issue Oct 6, 2022
Closed
@mattab mattab added the c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. label Dec 11, 2023
@mattab mattab added the c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. label Dec 11, 2023
@mattab mattab modified the milestones: 5.5.0, 5.4.0 Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github.
Projects
None yet
Milestone
5.4.0
Development

No branches or pull requests
5 participants
@tsteur @mattab @justinvelluppillai @sgiehl @atom-box