KnapsackPro / knapsack_pro-ruby

Knapsack Pro gem splits tests across parallel CI nodes and makes sure that tests run in optimal time
https://knapsackpro.com
MIT License
132 stars 28 forks source link

Add configuration to write to /tmp directory instead of .knapsack_pro #158

Open amckinnell opened 3 years ago

amckinnell commented 3 years ago

The change in 3.1.0 to use .knapsack_pro directory for temporary files instead of the tmp directory in the user's project directory is causing problems for me.

In the environment where I run tests I have the tmp directory configured to be writable and the root directory configured to be readonly.

Is it possible to add configuration to retain the behaviour prior to the changes in 3.1.0?

I imagine the default behaviour could be to use the .knapsack_pro directory with an optional configuration setting to explicitly set the directory or perhaps a flag to specify that the tmp directory should be used. Of course, adding this configuration makes my life simpler and your code more complicated.

ArturT commented 3 years ago

Hi @amckinnell

What do you mean by root directory? Is it your project directory (for example /home/project_repo/)?

What is your environment for running tests? Is this a business reason to not allow to write to your project directory?

knapsack_pro gem creates .knapsack_pro directory inside of your project directory like /home/project_repo/.knapsack_pro.

I moved on purpose knapsack_pro temporary files outside of user project's tmp directory because often people had code in tests that cleans up tmp folder and this was messing up with knapsack_pro behaviour.

Configuring your own path to a temporary directory other than .knapsack_pro could be a feature but maybe it's easier to just change the config on your CI and allow write files to .knapsack_pro directory?

Even if there was a way to configure a custom path to .knapsack_pro directory then what would you prefer to set it to? Is it tmp/.knapsack_pro? Then there is a risk that your tests could accidentally remove knapsack_pro temp files at some point in the future if someone won't be careful. If you use a different path than tmp/.knapsack_pro then you still need to tell your CI to allow write to that new directory.

amckinnell commented 3 years ago

Thanks. I will have to do some digging before I can answer your questions (mostly because I am not the designer of the CI environment). I think your decision to move the knapsack_pro files out of the tmp directory is wise. I just have to figure out what I need to change on my end. Cheers.

amckinnell commented 3 years ago

Solved the problem by changing the directory permissions and making the .knapsack_pro writable.

There are minimal advantages to making the project directory read-only. Thanks for your help and your product. Cheers.

ArturT commented 3 years ago

I'm glad you solved the issue. :)

charles-ferguson commented 3 years ago

I know this issue was already closed but I find this new behaviour to be surprising and would also appreciate you reconsidering your decision. We also had to alter our CI config to enable this change.

Temp files going to the tmp directory is the expected norm. This breaking because someone has configured hooks to clear tmp directories on spec runs feels like something they should be less surprised about than stashing these files under some directory we create from wherever knapsack is run.

ArturT commented 3 years ago

Hi @charles-ferguson and thank you for sharing the info. I was not aware that this change to keep files in project_dir/.knapsack_pro instead of project_dir/tmp/knapsack_pro could cause some problems.

Could you tell me more about what error did you see? Is it also related to permission access? I'm wondering why is that. What environment do you use? What CI provider? Do you use docker? How did you solve the issue? What have you added to your CI pipeline steps to make it work?

I was inspired by the fact that many CI providers use directories like .circleci, .buildkite, .github in the Rails project directory. So I decided to use .knapsack_pro directory in your project directory.

Temp files going to the tmp directory is the expected norm.

I'm wondering maybe instead of using Rails tmp directory we could use /tmp in OS. But this assumes you run knapsack_pro in UNIX environment. Or maybe knapsack_pro could check if /tmp directory exists before using it.

Do you have any other ideas about what we could try?

I don't know what was the root issue that you had. Would using /tmp in OS could also cause the same issue for you?

Another idea, maybe during the installation of knapsack_pro we should ask users to create .knapsack_pro directory within their project folder. This way the directory would exist and be committed into repo and tmp files could be written to it later on. (or maybe that's a false assumption and there would still be an issue with writing new files to such preexisting project_dir/.knapsack_pro directory)?

This breaking because someone has configured hooks to clear tmp directories on spec runs feels like something they should be less surprised about than stashing these files under some directory we create from wherever knapsack is run.

I was also hoping that people should be less surprised but I saw many times people frustrated that knapsack_pro was not work when project_dir/tmp/knapsack_pro directory was used. The problem was with missing files in project_dir/tmp/knapsack_pro because RSpec hooks or someone in the spec wrote code to clean up project_dir/tmp. This was hard to find and time-consuming and frustrating for users. People assume whatever is in project_dir/tmp could be removed. There were even cases when knapsack_pro was working fine but some times later someone wrote new tests and started removing files from project_dir/tmp and this caused knapsack_pro not to work as expected.

I'd appreciate your feedback so we could make a better decision about how to improve knapsack_pro gem. Thank you.

lckevin27 commented 2 years ago

Not sure if related, but I will go ahead and post a similar issue I had anyway... Upgrading to 3.1.0 actually worked fine for me, but there was some permission issue while deleting a file in the /tmp directory after I set KNAPSACK_PRO_FIXED_QUEUE_SPLIT to true in my Docker compose YAML file.

The error I was having on Buildkite was:

Failure/Error: File.delete(path) if File.exist?(path)

Errno::EPERM:
  Operation not permitted @ apply2files - /tmp/MY_FILE_NAME

It worked without a problem before configuring the KNAPSACK_PRO_FIXED_QUEUE_SPLIT option.

ArturT commented 2 years ago

Hi @lckevin27

This looks like a related issue. Can you share the full error from your CI here or over to support if it contains sensitive data https://knapsackpro.com/contact ?

I'm wondering why there is the path /tmp/MY_FILE_NAME. We don't write to that file nor delete it. Only to your project directory project_dir/.knapsack_pro.

Have you solved the issue? How did you solve it? I guess it is somehow specific to Docker but I'm not that familiar with Docker. Is your project mounted as a volume or copied inside of the docker container? Maybe this could cause a problem and the project is only for reading and nothing can be written to it so temp files can't be deleted.

ArturT commented 1 year ago

story

For internal tracking of this issue: https://trello.com/c/su0Oos27