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

fix(cypress): Check for local changes before trying to apply them #45889

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

SystemKeeper
Copy link
Contributor

@SystemKeeper SystemKeeper commented Jun 15, 2024

Summary

Running a chown command inside of docker on a machine that has CONFIG_OVERLAY_FS_METACOPY disabled takes unnecessarily long. In case of cypress, we try to apply local changes and run a chown 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:
image

TODO

According to the comment at

/**
* Applying local changes to the container
* Only triggered if we're not in CI. Otherwise the
* continuous-integration-shallow-server image will
* already fetch the proper branch.
*/
export const applyChangesToNextcloud = async function() {

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

@SystemKeeper SystemKeeper force-pushed the fix/noid/cypress-local-changes branch from aa3d796 to 7d62455 Compare June 15, 2024 17:46
Signed-off-by: Marcel Müller <marcel-mueller@gmx.de>
@SystemKeeper SystemKeeper force-pushed the fix/noid/cypress-local-changes branch from 062ae5e to a5ba81a Compare June 15, 2024 18:20
@SystemKeeper SystemKeeper changed the title Test cypress local changes fix(cypress): Check for local changes before trying to apply them Jun 15, 2024
@SystemKeeper SystemKeeper added 3. to review Waiting for reviews CI labels Jun 15, 2024
@SystemKeeper SystemKeeper marked this pull request as ready for review June 15, 2024 18:31
@SystemKeeper SystemKeeper self-assigned this Jun 17, 2024
@st3iny
Copy link
Member

st3iny commented Jun 17, 2024

Why not disable the check entirely on CI?

See also #45894

@SystemKeeper
Copy link
Contributor Author

Why not disable the check entirely on CI?

Yea, see above:

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?!

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?

@solracsf solracsf added this to the Nextcloud 30 milestone Jun 18, 2024
@st3iny
Copy link
Member

st3iny commented Jun 18, 2024

I agree, let's have both PRs then.

@skjnldsv skjnldsv merged commit ae4a64b into master Jun 18, 2024
113 checks passed
@skjnldsv skjnldsv deleted the fix/noid/cypress-local-changes branch June 18, 2024 13:11
@susnux
Copy link
Contributor

susnux commented Jul 2, 2024

This breaks local testing. You are now forced to have your nextcloud repository in '/var/www/html' because otherwise it will always skip uploading.

@SystemKeeper
Copy link
Contributor Author

But the path was /var/www/html before that PR already or did I mis-interpreted the old code ?

@susnux
Copy link
Contributor

susnux commented Jul 2, 2024

It is uploaded to /var/www/html on the docker image, but you are checking if the local path is /var/www/html/something which is not the case (probably)

@SystemKeeper
Copy link
Contributor Author

Sorry, I mis-interpreted the tar command... Was going to take a look at this, but seems it was already fixed in #46370 - correct?

@susnux
Copy link
Contributor

susnux commented Jul 15, 2024

Yes

@blizzz blizzz mentioned this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants