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

Cypress deletes screenshotsFolder and videosFolder, loosing their original permissions #1943

Closed
kamituel opened this Issue Jun 14, 2018 · 11 comments

Comments

6 participants
@kamituel
Copy link

kamituel commented Jun 14, 2018

Current behavior:

When Cypress is launched using cypress run with the trashAssetsBeforeRuns=true, it will delete (move to trash, to be more precise) the whole screenshotsFolder and videosFolder. After re-creating them, they'll have different access control settings (file system-wise) then originally.

See "Steps to reproduce" for details and why it's an issue.

I realize it's an exotic issue, and I can work around it fairly easily without fixing Cypress. So feel free to close if you don't consider this to be a bug.

Desired behavior:

Cypress deletes the contents of screenshotsFolder and videosFolder directories, but not the directories themselves. Thus, it preserves their filesystem owner settings, and rwx settings.

Cypress has an option to permanently delete those files, not only move them to trash (to avoid similar issue with the .Trash directory itself)

Steps to reproduce:

  1. CI agent is deleting /artifacts directory.
  2. CI agent is recreating this directory, as a non-privileged users, so that it looks like:
artifacts/
    - screenshots/
    - videos/
  1. Cypress, running in a Docker container, is executing a test as a root user. It has access to the /results folder because it's mounted as a Docker volume. It deletes screenshots/ and videos/ and re-creates them.

  2. Test passes.

  3. Next test is about to run on the very same agent. Agent fails to delete /results/videos/lorem.mp4 - it has no write access to videos/ and screenshots/, because they were created as root.

Versions

Running in Docker, on Linux. Custom setup, not based on Cypress' docker images.

@dominics

This comment has been minimized.

Copy link

dominics commented Jul 19, 2018

I'm seeing something similar when using volumes in Docker (so that I can access the screenshots/videos after a test run inside the container is complete). My Docker Compose setup looks something like (mine are based on Cypress' docker images):

services:
  cypress:
    // [...]
    volumes:
      - "./cypress/reports:/app/cypress/reports"
      - "./cypress/screenshots:/app/cypress/screenshots"

In this setup, because those volumes are mountpoints, deleting them fails with an EBUSY:

Warning: We failed to trash the existing run results.
--
  |  
  | This error will not alter the exit code.
  |  
  | Error: EBUSY: resource busy or locked, rmdir '/app/cypress/screenshots'

The workaround is, of course, to change the screenshotsFolder/videosFolder to be under some volume mount, rather than at the volume mount.

Slightly different scenario and error. But this, too, would be fixed if Cypress was deleting the contents of those directories, instead of the directories themselves.

@jennifer-shehane

This comment has been minimized.

Copy link
Member

jennifer-shehane commented Jul 19, 2018

Here is the method that runs to trash the folders which could be updated:

https://github.com/cypress-io/cypress/blob/develop/packages/server/lib/modes/run.coffee#L319

There's likely tests that would need to be updated also.

@kamituel

This comment has been minimized.

Copy link
Author

kamituel commented Jul 25, 2018

@jennifer-shehane is this bug on a roadmap to be fixed any time soon?

If not, I might help with that. What kind of fix we want for this - remove the contents of videos and screenshots directories permanently?

@brian-mann

This comment has been minimized.

Copy link
Member

brian-mann commented Jul 25, 2018

@kamituel

Likely want to glob inside of each folder and remove the folders / files in there as opposed to the direct parent.

I'm not sure if this would even fix the problem though because where does it end? It would just remove the permissions on those folders.

@dominics

Will it work deleting children out of a folder that is mounted? Has anyone tried that?

/all
You could just turn off trashing assets before runs in CI. We already support configuration for that. That is likely the easiest path here and if CI runs on a clean instance there won't be anything in there anyway.

cypress run -c trashAssetsBeforeRuns=false
@kamituel

This comment has been minimized.

Copy link
Author

kamituel commented Jul 25, 2018

Yeah, I kind of see this as an integration issue between Cypress and our CI process (which is arguably complicated). But at the end of the day, it is Cypress who is pointed to an existing directory and it's changing that directorie's permissions / ownership. So I believe it should be fixed within Cypress.

Philosophy aside, disabling trashing assets before runs wouldn't solve it for us, because the issue is that our scripts running on an agent (not in a docker container) break because rm -r /results/screenshots fails.

We tried using TRAP's in a shell script running in an agent to ensure we chmod 666 those directories, but traps didn't seem to fire correctly on Bamboo.

If anything, we could just leave those assets on an agent and let the container in the next build clean it up before (so keep trashing assets before runs enabled). That'd mean we'd have stale artifacts laying around on agents, which I dislike, but it's an option if we can't fix that in Cypress.

@brian-mann

This comment has been minimized.

Copy link
Member

brian-mann commented Jul 25, 2018

@kamituel and this would be fixed if we just globbed and removed the stuff underneath those directories yah?

If so, we can do that, pretty easy fix.

@kamituel

This comment has been minimized.

Copy link
Author

kamituel commented Jul 25, 2018

Yes, but assuming we'll delete files permanently (as in rm), and not use trash npm module Cypress currently uses. That's because on Linux it makes a .Trash directory next to where the deleted files were originally located, and this directory has incorrect permissions / ownership too.

@CGastrell

This comment has been minimized.

Copy link

CGastrell commented Aug 6, 2018

Hi everyone, I stumbled upon some very similar scenario and I was wondering: why trash the complete directory? Why not rm -rf {videosFolder}/* (idem for screenshots).

I know it's safer/cleaner to rm the whole dir but that way also messes with .gitignore strategies.

To be specific: I want to keep the folders (videos & screenshots) so I don't have to worry about not existing/permissions/etc. So I keep a .gitignore inside those dirs, but as the trashing gets rid of the whole dir, my strategy also gets trashed.

I think I'm suggesting the same as @brian-mann which, in the end, points to not messing with directory structure of the project

Besides personal or coding strategies, I think trashing the folders will continue to raise problems on different scenarios.

@lilaconlee

This comment has been minimized.

Copy link
Contributor

lilaconlee commented Sep 17, 2018

@kamituel I've got the globbing fix PR'd but sounds like that'll still cause issues for you with .Trash.

From digging around in the Trash spec, seems like a work around could be creating $topdir/.Trash yourself. Lemme know what you think, tried testing this out on Ubuntu but couldn't replicate the .Trash behavior to begin with.

@kamituel

This comment has been minimized.

Copy link
Author

kamituel commented Oct 18, 2018

Thanks @lilaconlee! Once this gets released, I'll test it, and the $topdir/.Trash workaround, on our CI.

@lilaconlee

This comment has been minimized.

Copy link
Contributor

lilaconlee commented Nov 14, 2018

Hey @kamituel, some more issues popped up with trashing so we opted to permanently delete the contents of directories in Linux as you mentioned. This was done in #2745 and will be released with 3.1.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.