-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(cypress): Check for local changes before trying to apply them #45889
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
Conversation
aa3d796 to
7d62455
Compare
Signed-off-by: Marcel Müller <marcel-mueller@gmx.de>
062ae5e to
a5ba81a
Compare
|
Why not disable the check entirely on CI? See also #45894 |
Yea, see above:
If it doesn't serve any purpose, we should disable it completely on CI. But I think this change here might even make sense when not running on CI? |
|
I agree, let's have both PRs then. |
|
This breaks local testing. You are now forced to have your nextcloud repository in '/var/www/html' because otherwise it will always skip uploading. |
|
But the path was |
|
It is uploaded to |
|
Sorry, I mis-interpreted the tar command... Was going to take a look at this, but seems it was already fixed in #46370 - correct? |
|
Yes |
Summary
Running a
chowncommand inside of docker on a machine that hasCONFIG_OVERLAY_FS_METACOPYdisabled takes unnecessarily long. In case of cypress, we try to apply local changes and run achowncommand afterwards, even when no files were actually changed/added.This PR checks if any of the files exist, before copying them to the container and executing a
chowncommand.Example:

TODO
According to the comment at
server/cypress/dockerNode.ts
Lines 124 to 130 in 8359b80
This method was never intended to run on CI, but it seems to be the case since this method was introduced (#34696). It might make sense to just check for
process.env.CIand never execute that method, but I am not sure if it serves a purpose for something on CI?!Checklist