-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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(cypress): Check for local changes before trying to apply them #45889
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
chown
command inside of docker on a machine that hasCONFIG_OVERLAY_FS_METACOPY
disabled takes unnecessarily long. In case of cypress, we try to apply local changes and run achown
command 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
chown
command.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.CI
and never execute that method, but I am not sure if it serves a purpose for something on CI?!Checklist